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

[ENHANCEMENT] Upgrade ESLint to v8 in app and addon blueprints #9669

Closed
wants to merge 15 commits into from

Conversation

nightire
Copy link
Contributor

@nightire nightire commented Oct 18, 2021

babel-eslint does not works well with eslint@8, this PR uses @babel/eslint-parser instead.

The failing tests are not related with changes in this PR.

It's required to use `@babel/plugin-proposal-decorators` plugin with
`@babel/eslint-parser` if we want to support decorators in JavaScript.

This reverts commit 8ae8d5f.
@SergeAstapov
Copy link
Contributor

@nightire I assume this is similar to #9556 and last comment from @rwjblue in #9556 is that we need an RFC for such a change.

cc @bmish as you worked originally on #9556.

I can volunteer to start an RFC to the extent my tech writing allows me to do, just to start the bowl rolling

@nightire
Copy link
Contributor Author

@SergeAstapov that would be awesome, thank you.

@miguelcobain
Copy link

ember-auto-import doesn't seem happy with useBabelConfig: true: embroider-build/ember-auto-import#471

@@ -23,14 +23,15 @@
"test:ember": "ember test"
},
"devDependencies": {
"@babel/eslint-parser": "^7.15.8",
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a way we can bundle these dependencies as fallbacks? so we don't have to bundle them in the app's package.json?

Copy link
Contributor

Choose a reason for hiding this comment

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

no

Copy link
Contributor

Choose a reason for hiding this comment

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

There is! I found a way

jacobq added a commit to jacobq/repro-ember-drag-drop-on-ember4-embroider-app that referenced this pull request Feb 28, 2022
Upgrading it seems to break stuff with babel-parser. See:

* ember-cli/ember-cli#9307
* ember-cli/ember-cli#9669
@bertdeblock
Copy link
Contributor

Opened emberjs/rfcs#817 to hopefully get the ball rolling on this one.

andreyfel added a commit to retailnext/ember-bem-helpers that referenced this pull request May 18, 2022
Get rid of deprecated `babel-eslint`
References:
ember-cli/ember-cli#9669
ijlee2/ember-container-query#115
@ef4
Copy link
Contributor

ef4 commented Jul 6, 2022

I don't think upgrading the parser like this needs an RFC.

However, my main concern is that this hard-codes a separate babel config for use by eslint that needs to be maintained separately from the real config. That might be an acceptable tradeoff given that it will be more difficult for many apps to quickly adopt a standalone babel config file.

@wagenet
Copy link
Contributor

wagenet commented Jul 23, 2022

@ef4 what would it take for you to feel comfortable landing this?

@jelhan
Copy link
Contributor

jelhan commented Oct 4, 2022

What is the status here? Would be great if we could upgrade to eslint v8 in Ember projects. In general migration from babel-eslint to @babel/eslint-parser seems to desirable on its own as well was babel-eslint is deprecated since a long-time.

@bertdeblock
Copy link
Contributor

I would like to see this progress as well. I'll try to update the PR and put this on the agenda for the CLI meeting tomorrow.

@SergeAstapov
Copy link
Contributor

@bertdeblock thanks for helping with this! If RFC still needed, I can help with it as called out before however there were several comments here and there it may not be needed.

@bmish
Copy link
Member

bmish commented Oct 4, 2022

I agree it's important to get this ESLint / @babel/eslint-parser upgrade in.

Last I saw, it was stalled on comments on this PR / my previous PR attempt (#9556).

Note that Ember already works fine with these upgraded versions, they just don't come standard in the Ember blueprint yet.

blueprints/app/files/package.json Show resolved Hide resolved
"eslint-config-prettier": "^8.3.0",
"eslint-plugin-ember": "^10.4.2",
"eslint-plugin-ember": "^10.5.4",
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems outdated, don't think this version should be touched for this PR?

@bertdeblock bertdeblock changed the title upgrade eslint to v8 [ENHANCEMENT] Upgrade ESLint to v8 in app and addon blueprints Oct 5, 2022
@bertdeblock bertdeblock self-assigned this Oct 5, 2022
@ef4
Copy link
Contributor

ef4 commented Oct 5, 2022

While I still don't love having a separate babel config in the eslintrc, I think it's the closest way to configure the existing behavior in the new version. So I'm ok with it.

@ijlee2
Copy link
Contributor

ijlee2 commented Dec 1, 2022

Hello, I think it'd be good for us to resume working on this pull request and getting it merged soon.

To provide confidence to this PR: I was able to migrate a monorepo with addons and engines to use eslint@v8.28 (with a mix of @babel/eslint-parser and @typescript-eslint/parser, as well as using eslint-plugin-n instead of eslint-plugin-node). I would like to see an official support of eslint@v8 in the blueprints.

Going forward, eslint will ask for a flat configuration. We'll also want to work together on updating eslint-plugin-ember to be compatible with the new format.

PS. In eslint/eslint#13481, how can we get eslint-plugin-ember listed under Phase 3: Compatibility testing to raise awareness?

@bmish
Copy link
Member

bmish commented Dec 1, 2022

PS. In eslint/eslint#13481, how can we get eslint-plugin-ember listed under Phase 3: Compatibility testing to raise awareness?

I don't think we need the ESLint team to monitor compatibility with eslint-plugin-ember. They just listed a few top ESLint plugins in that section as a sanity check that their flat config implementation works properly. But it would be good to file an issue in eslint-plugin-ember for us to add support for flat config.

@bmish
Copy link
Member

bmish commented Dec 1, 2022

@nightire can you merge the latest master into this PR? As @ef4 said, I think it's time we proceed with this PR, even if we're not perfectly happy with the separate babel config inside the ESLint config. We can adjust that setup later, but it's important we get ESLint updated now.

@NullVoxPopuli
Copy link
Contributor

For reference, i have a flat config here: embroider-build/addon-blueprint#71

@nightire
Copy link
Contributor Author

nightire commented Dec 3, 2022

@nightire can you merge the latest master into this PR? As @ef4 said, I think it's time we proceed with this PR, even if we're not perfectly happy with the separate babel config inside the ESLint config. We can adjust that setup later, but it's important we get ESLint updated now.

@bmish Done.

"eslint-config-prettier": "^8.3.0",
"eslint-plugin-ember": "^10.4.2",
"eslint-plugin-ember": "^10.5.4",
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem to be properly rebased. This is an older version than is on master:

"eslint-plugin-ember": "^11.1.0",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With a few searches and replace, all occurrences are now updated to the ^11.10.

@bertdeblock
Copy link
Contributor

The parser in the ESLint config file is still babel-eslint and babel-eslint is still included in package.json. Note that we now also need to take the typescript flag into account, because ts projects use a different parser + parser options.

@bmish
Copy link
Member

bmish commented Dec 3, 2022

Can you search for "eslint" to make sure that it's bumped to the same v8 version everywhere? There are still fixtures it hasn't been updated in.

@bmish
Copy link
Member

bmish commented Dec 3, 2022

As mentioned by @bertdeblock, I still see babel-eslint in a bunch of places.

@@ -24,6 +24,8 @@
"test:ember": "ember test"
},
"devDependencies": {
"@babel/eslint-parser": "^7.15.8",
Copy link
Contributor

@ijlee2 ijlee2 Dec 4, 2022

Choose a reason for hiding this comment

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

bug (blocking)

Before, babel-eslint was added to package.json via the else clause. You'll want to add @babel/eslint-parser and @babel/plugin-proposal-decorators conditionally.

Pseudocode:

<% if (!typescript) { %>
"@babel/eslint-parser": "^7.19.1",
"@babel/plugin-proposal-decorators": "^7.20.5",
<% } %>

I listed the latest versions above, in case you want the latest.

@@ -54,7 +55,7 @@
"ember-source": "~4.9.0-beta.1",
"ember-template-lint": "^4.16.1<% if (welcome) { %>",
"ember-welcome-page": "^6.2.0<% } %>",
"eslint": "^7.32.0",
"eslint": "^8.24.0",
Copy link
Contributor

@ijlee2 ijlee2 Dec 4, 2022

Choose a reason for hiding this comment

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

The latest version is 8.29.0, in case you want the latest.

(We can always create a separate PR to update eslint and its plugins to the latest.)

Comment on lines +14 to +15
plugins: [
['@babel/plugin-proposal-decorators', { decoratorsBeforeExport: true }],
Copy link
Contributor

Choose a reason for hiding this comment

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

question

Now that @babel/plugin-proposal-decorators is used, I don't think we need the ecmaFeatures key (along with legacyDecorators) anymore (lines 9-11)?

The first commit (e97c6ad) shows that ecmaFeatures had been removed. I wonder if it got accidentally re-introduced by merging master (i.e. a rebase didn't happen correctly).

@@ -143,7 +143,7 @@
"chai-jest-snapshot": "^2.0.0",
"ember-cli-blueprint-test-helpers": "^0.19.2",
"ember-cli-internal-test-helpers": "^0.9.1",
"eslint": "^8.24.0",
"eslint": "^8.29.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Bumping eslint for ember-cli itself can be omitted from this PR imo, as it's not related to the blueprints.

@@ -24,6 +24,8 @@
"test:ember": "ember test"
},
"devDependencies": {
<% if (!typescript) { %>"@babel/eslint-parser": "^7.19.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the file should still be valid JSON, see #10058 for more info.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bertdeblock Do you have the permissions to run CI? It'd help @nightire to see if linting and testing are passing. (For first-time contributors, I'm not sure if you may need to approve running workflows after every push.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Approved, but it's failing because eslint was bumped in ember-cli's package file without updating the lock file.
I would omit that bump from this PR though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet