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

chore: switch to true static attributes #3989

Merged
merged 1 commit into from
Nov 10, 2021

Conversation

wraithgar
Copy link
Member

now that we're off of node10 we can clean this up

@wraithgar wraithgar requested a review from a team as a code owner November 3, 2021 22:21
@isaacs
Copy link
Contributor

isaacs commented Nov 3, 2021

Probably not a concern in this case, but something to be aware of when switching from static get foo () { return 'bar' } to static foo = 'bar' is that it's no longer write-protected.

@wraithgar
Copy link
Member Author

We can't move to this until we move onto @npmcli/eslint-config which is a larger task than just switching, because there are several bugs that were fixed there that aren't fixed here, such as line length enforcement being ignored in almost 250 places here.

@wraithgar
Copy link
Member Author

Probably not a concern in this case, but something to be aware of when switching from static get foo () { return 'bar' } to static foo = 'bar' is that it's no longer write-protected.

Not in the constructor, but it's protected in the "blessed" api i.e. in the instantiated object.

https://github.com/npm/cli/blob/latest/lib/base-command.js#L12-L18

If someone requires the base class and overrides the constructor there that's ... on them?

@wraithgar wraithgar marked this pull request as draft November 4, 2021 14:20
@wraithgar wraithgar force-pushed the gar/static-attributes branch from f1eeb6a to 0797274 Compare November 4, 2021 22:06
@darcyclarke darcyclarke added semver:patch semver patch level for changes Release 8.x work is associated with a specific npm 8 release labels Nov 4, 2021
@ljharb
Copy link
Contributor

ljharb commented Nov 4, 2021

why do you need to wait for the eslint config? you should be able to set the parserOptions.ecmaVersion to 2022 in the meantime.

@wraithgar
Copy link
Member Author

wraithgar commented Nov 4, 2021

eslint@7 doesn't support it at all, and updating to 8 in this repo would have meant auditing the other eslint plugins and updating this local eslint config, something that had already been done in our linting repo. We already moved the linting today so it's just a matter of landing the breaking change to @npmcli/eslint-config

@wraithgar
Copy link
Member Author

npm/eslint-config#10

@ljharb
Copy link
Contributor

ljharb commented Nov 4, 2021

ah, true, you'd need to use @babel/eslint-parser with eslint 7.

@isaacs
Copy link
Contributor

isaacs commented Nov 8, 2021

@wraithgar

Not in the constructor, but it's protected in the "blessed" api i.e. in the instantiated object.

Ah, ok, I'd missed that. Since we don't support loading npm programmatically any more in v8, I really can't see any problem. It's easy enough for us to just not do the wrong thing.

@wraithgar wraithgar force-pushed the gar/static-attributes branch from 0797274 to 80f2568 Compare November 8, 2021 16:39
@wraithgar wraithgar marked this pull request as ready for review November 8, 2021 16:40
@wraithgar wraithgar force-pushed the gar/static-attributes branch 2 times, most recently from b5112de to 6bdd183 Compare November 8, 2021 16:53
@wraithgar wraithgar removed the semver:patch semver patch level for changes label Nov 8, 2021
now that we're off of node10 we can clean this up

PR-URL: #3989
Credit: @wraithgar
Close: #3989
Reviewed-by: @lukekarrys
@wraithgar wraithgar force-pushed the gar/static-attributes branch from 6bdd183 to ea352f5 Compare November 10, 2021 22:50
@wraithgar wraithgar merged commit ea352f5 into release-next Nov 10, 2021
@wraithgar wraithgar deleted the gar/static-attributes branch November 10, 2021 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release 8.x work is associated with a specific npm 8 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants