-
Notifications
You must be signed in to change notification settings - Fork 10
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
Run linting on the generated project in CI // add a working eslintconfig #84
Conversation
This has revealed that the upstream blueprint's eslintconfig is invalid. I will be updating that in this PR |
5176adc
to
9bca726
Compare
...ensureLatestDeps, | ||
// Linting | ||
'@babel/plugin-proposal-decorators', | ||
].filter((depToRemove) => existingDeps.includes(depToRemove)), |
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.
because of this filter, there is no danger to accidentally specifying something that isn't present in the upstream blueprint.
|
||
...ensureLatestDeps, | ||
// Linting | ||
'@babel/plugin-proposal-decorators', |
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.
Unlike upstream, we don't want this plugin because we're using decorator-transforms
in our babel config.
@@ -124,5 +149,16 @@ module.exports = { | |||
}); | |||
|
|||
await this.updateDeps(options); | |||
|
|||
const lintFix = require('ember-cli/lib/utilities/lint-fix'); |
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.
lint:fix wasn't running at all before and now it is (and is kinda required, since version drift could change how the output of files end up)
/** | ||
* ESM node files | ||
*/ | ||
{ |
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.
this section isn't present in the upstream blueprint, because we have a vite.config.mjs and now eslint.config.mjs
import emberParser from 'ember-eslint-parser'; | ||
import babelParser from '@babel/eslint-parser'; | ||
|
||
const esmParserOptions = { |
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.
unlike the upstream blueprint, we have a babel config, so we don't need to specify one here
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.
LGTM!
Slightly off-topic, but do I recall correctly from yesterday's team meeting, that we have agreement to make this blueprint independent from the official one? I feel that layering approach is doing more harm here than good, with all those overriding, installing additional and deinstalling unwanted deps. That can get confusing to maintain and brittle, at least that has been my experience with similar blueprint approaches.
Yes! I agree -- to prove that point (because not everyone agreed), I'd like to get all the way to TS support, as in landing #80, and then separating the blueprints -- which, I suspect will make for a much less weird index.js! |
Unblocks CI verifying that #83 is correct