-
Notifications
You must be signed in to change notification settings - Fork 6
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
feat: config lwc mobile eslint plugin #68
Conversation
const eslintRecommended = 'eslint:recommended'; | ||
const config = workspace.getConfiguration(); | ||
|
||
class EslintDependencyConfig { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
98b19fd
to
fcbd329
Compare
fcbd329
to
6ac6338
Compare
@salesforce/eslint-plugin-lwc-mobile 1.0.0 is published. so this pr is ready :) |
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