-
-
Notifications
You must be signed in to change notification settings - Fork 41
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
Updated import-meta-resolve
& pack it automatically
#87
Conversation
.vscode | ||
converted-esm/ |
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.
I'd prefer committing it to the repo, so it can be upgraded only when I run npm run convert-esm
.
just pushed a commit to do it: 65f6b45, can you cherry pick it to your branch?
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.
I don't like the idea of including generated code in a git repo, but sort of I understand the reasons, but what about the removal of the prepare
script? This way we are sure it's being used the same version that's defined in package.json
and not an old one because we forgot to generate it by hand... What do you think?
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.
If we remove the prepare
, a maintainer may forget to run the convert-esm
before publishing, so pushing a broken package to npm.
I think the safest way is to commit it in git, but provide a straightforward way to upgrade it, like:
npm i import-meta-resolve@latest -D
npm run convert-esm
git add ./convert-esm
git commit -m "chore: upgrade import-meta-resolve"
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.
another point: it allows users to test an unpublished version from github:
npm i https://github.com/eslint-community/eslint-plugin-n#main
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.
That doesn't makes sense. It could if we would have any modificación, but we are just transpiling It, so it's a verbatin copy and almost the same as if we would be using It from node_modules, so let's stick with the same workflow. Prepare script is run both when publising to npm registro, and when installing from git repositorio, so it's the same you are proposing. If you want to have a cozy on the git repo, it's superfluos and I disagree but that's fine in that case we should add a git hook to be sure it's being included the latests versión but that's cumbersome. What's mandatory is to generate It in the prepare script to be sure ALWAYS is being used the same versión defined on package.json, and prevent somebody forget to update 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.
We can pin it in the package.json
file, there's no need to do anything else, everything can be safely automated.
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.
it also makes sense! 👍
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.
ok, next steps to get this merged and published?
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.
will do it later. :)
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.
thank you :-)
The v2 version uses the prefixed "node: assert", which is not supported on node.js v12, while the plugin still supports. I think we could re-introduce it in the next major, and I'm going to revert it for now. thoughts? |
…)" This reverts commit f74d35e.
According to https://nodejs.org/api/esm.html#node-imports, Node.js v12.20 supports |
we had a plan to drop node.js<14 in next major version: see #42 and yes, we could consider drop <16. 👍 |
As this is planned to fix #81, then I think some unit test should be added for usage with the |
There's no need for explicit tests, just import them. |
@thernstig means: good to have a few tests in here: eslint-plugin-n/tests/lib/rules/prefer-promises/fs.js Lines 19 to 29 in 1fc0bf6
to make sure it's working as expected and avoid potential regressions. 😄 |
Oh! makes sense :-) |
Any update on this? |
it has been landed in aa75610, I'll try to publish it soon. |
Glad to know, thank you. |
Fix #86