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

Upgrade addon to eslint 8 #108

Merged
merged 4 commits into from
May 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions files/__addonLocation__/.eslintrc.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,6 @@ module.exports = {
parserOptions: {
ecmaVersion: 'latest',<% if (!typescript) { %>
sourceType: 'module',
ecmaFeatures: {
legacyDecorators: true,
},
babelOptions: {
root: __dirname,
},<% } %>
Expand Down Expand Up @@ -38,10 +35,10 @@ module.exports = {
<% } %> // node files
{
files: [
'./.eslintrc.js',
'./.eslintrc.cjs',
Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli Feb 10, 2023

Choose a reason for hiding this comment

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

what about *.cjs?
and .prettierrc.js should be cjs, too

Copy link
Collaborator

Choose a reason for hiding this comment

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

For reference, I suppose, this is how I made my "One lint config" for all apps, addons, v2 addons: https://github.com/NullVoxPopuli/eslint-configs/blob/main/configs/ember.js#L12

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what about *.cjs? and .prettierrc.js should be cjs, too

only the files that I've touched here are delivered from this repo (eg https://github.com/embroider-build/addon-blueprint/blob/main/files/__addonLocation__/.eslintrc.cjs), the rest seem to be from the base addon
as far as I've generated the addon with this command it creates a .prettierrc.js and not a .cjs file hence I've left that as is
the changes here are correct as for the output of this blueprint (to the best of my ability to verify)

not sure what if anything you expect to be different here

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's just that if we throw type=module in the package.json, the prettier file will be required to be cjs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just that if we throw type=module in the package.json, the prettier file will be required to be cjs

these are regex's so how about I make all options valid?

Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli Feb 11, 2023

Choose a reason for hiding this comment

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

How do you mean? All the 'node files' should be .cjs, and then this eslint entry can be *.cjs instead of the 4 or 5 lines it is now. But, i think bucause all our browser code is in src, we could probably safely assume all *.{cjs,js} are node. The toplevel cjs in def node, and the top level js would be esm. One eslint entry to rule them all! (Of the package root lol)

Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli Feb 11, 2023

Choose a reason for hiding this comment

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

(tho, because parserType is 'script', only cjs is supported atm -- kinda obnoxious that esm and cjs need different parsers.)

'./.prettierrc.js',
'./.template-lintrc.js',
'./addon-main.js',
'./.template-lintrc.cjs',
'./addon-main.cjs',
],
parserOptions: {
sourceType: 'script',
Expand Down
5 changes: 5 additions & 0 deletions files/__addonLocation__/.prettierrc.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
'use strict';

module.exports = {
singleQuote: true,
};
2 changes: 1 addition & 1 deletion files/__addonLocation__/babel.config.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
<% } %> "plugins": [<% if (typescript) { %>
["@babel/plugin-transform-typescript", { "allowDeclareFields": true }],<% } %>
"@embroider/addon-dev/template-colocation-plugin",
["@babel/plugin-proposal-decorators", { "legacy": true }],
["@babel/plugin-proposal-decorators", { "decoratorsBeforeExport": true }],
Copy link
Contributor

@ijlee2 ijlee2 May 30, 2023

Choose a reason for hiding this comment

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

@void-mAlex @NullVoxPopuli When setting decoratorsBeforeExport to true, does one need to configure their v2 addon somehow more?

Some tests for the docs and test apps for ember-container-query failed locally and in CI: https://github.com/ijlee2/ember-container-query/actions/runs/5124925949

# One type of error
Error: Assertion Failed: The options object passed to tracked() may only contain a 'value' or 'initializer' property, not both. Received: [kind,key,placement,descriptor,ini

# Another type
ReferenceError: Cannot access 'ContainerQueryComponent' before initialization

# And another
TypeError: An element descriptor's .kind property must be either "method" or "field", but a decorator created an element descriptor with .kind "undefined"

I couldn't find examples of other addons that currently use decoratorsBeforeExport: true.

https://emberobserver.com/code-search?codeQuery=decoratorsBeforeExport&fileFilter=babel

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah, this was probably a mistake, we should bring to legacy - can you PR an update? I have my release machine setup atm, so I can push it out pretty quick

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I will create a PR, thanks for the quick feedback.

Copy link
Contributor

Choose a reason for hiding this comment

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

"@babel/plugin-proposal-class-properties"
]
}
6 changes: 3 additions & 3 deletions files/__addonLocation__/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,9 @@
},
"devDependencies": {
"@babel/core": "^7.17.0",
<% if (typescript) { %>"@babel/preset-typescript": "^7.18.6"<% } else { %>"@babel/eslint-parser": "^7.18.2"<% } %>,
<% if (typescript) { %>"@babel/preset-typescript": "^7.18.6"<% } else { %>"@babel/eslint-parser": "^7.19.1"<% } %>,
"@babel/plugin-proposal-class-properties": "^7.16.7",
"@babel/plugin-proposal-decorators": "^7.17.0",
"@babel/plugin-proposal-decorators": "^7.20.13",
"@babel/plugin-syntax-decorators": "^7.17.0",
"@babel/runtime": "^7.17.0",
"@embroider/addon-dev": "^3.0.0",<% if (typescript) { %>
Expand Down Expand Up @@ -62,7 +62,7 @@
"@rollup/plugin-babel": "^6.0.3",<% } %>
"concurrently": "^8.0.1",
"ember-template-lint": "^5.7.3",
"eslint": "^7.32.0",
"eslint": "^8.33.0",
void-mAlex marked this conversation as resolved.
Show resolved Hide resolved
"eslint-config-prettier": "^8.3.0",
"eslint-plugin-ember": "^11.6.0",
"eslint-plugin-n": "^16.0.0",
Expand Down