Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: config lwc mobile eslint plugin #68

Conversation

ben-zhang-at-salesforce
Copy link
Contributor

Scaffolding up adding lwc-mobile eslint plugin into salesforcedx-vscode-mobile.

not fully read yet, need to wait for lwc-mobile eslint plugin release to npmjs.com

const eslintRecommended = 'eslint:recommended';
const config = workspace.getConfiguration();

class EslintDependencyConfig {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In TypeScript, these kinds of data structures are better represented as types (or type aliases, technically). I actually like their types cheat sheet, once your head is around the basic concept. Nice go-to reference for syntax.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, let me do that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

read a bit about type/type aliases, I didn't figure out a way to let type to have function as a member. I think class is still better due to we need to have version value to be dynamically set based. so uses class with a function getVersion() support it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it'd be a bit different in the type setup. Instead of having a "lazy load" method to get the version, you'd simply initialize the version with the logic that it contains. Arguably, that's the only reason the packageConfigPropertyId exists. So it would instead look something like this:

type EslintDependencyConfig = {
    name: string;
    version: string;
    eslintConfigToExtend: string;
};

const eslintDependencies: EslintDependencyConfig[] = [
    {
        name: '@salesforce/eslint-plugin-lwc-mobile',
        eslintConfigToExtend: 'plugin:@salesforce/lwc-mobile/recommended',
        version: config.get('mobileOfflineLinting.eslint-plugin-lwc-mobile')
    },
    // etc.
];

But it's not a big deal. It works fine the way you have it too.

Copy link
Collaborator

@khawkins khawkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few suggestions, but overall this looks fine to me.

@ben-zhang-at-salesforce
Copy link
Contributor Author

@salesforce/eslint-plugin-lwc-mobile 1.0.0 is published. so this pr is ready :)

@ben-zhang-at-salesforce ben-zhang-at-salesforce merged commit 0b07787 into salesforce:main Jun 28, 2024
9 checks passed
@ben-zhang-at-salesforce ben-zhang-at-salesforce deleted the bundleInLwcMobileEslint branch June 28, 2024 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants