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

Updated import-meta-resolve & pack it automatically #87

Merged
merged 1 commit into from Mar 3, 2023

Conversation

piranna
Copy link

@piranna piranna commented Mar 1, 2023

Fix #86

.vscode
converted-esm/

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?

Copy link
Author

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?

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"

Copy link

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

Copy link
Author

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.

Copy link
Author

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.

Choose a reason for hiding this comment

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

it also makes sense! 👍

Copy link
Author

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?

Choose a reason for hiding this comment

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

will do it later. :)

Copy link
Author

Choose a reason for hiding this comment

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

thank you :-)

@aladdin-add aladdin-add merged commit f74d35e into eslint-community:master Mar 3, 2023
@aladdin-add
Copy link

The v2 version uses the prefixed "node: assert", which is not supported on node.js v12, while the plugin still supports.
https://github.com/eslint-community/eslint-plugin-n/actions/runs/4319585453/jobs/7538932736

I think we could re-introduce it in the next major, and I'm going to revert it for now. thoughts?

@piranna
Copy link
Author

piranna commented Mar 3, 2023

The v2 version uses the prefixed "node: assert", which is not supported on node.js v12, while the plugin still supports.
eslint-community/eslint-plugin-n/actions/runs/4319585453/jobs/7538932736

According to https://nodejs.org/api/esm.html#node-imports, Node.js v12.20 supports node: prefixed modules. In any case, Node.js v12 is EOL, and the same will happen with Node.js v14 in less than two months, so in fact we should upgrade the base requirements to Node.js v16.

@aladdin-add
Copy link

we had a plan to drop node.js<14 in next major version: see #42

and yes, we could consider drop <16. 👍

@thernstig
Copy link

thernstig commented Mar 10, 2023

As this is planned to fix #81, then I think some unit test should be added for usage with the node: prefix?

@piranna
Copy link
Author

piranna commented Mar 10, 2023

As this is planned to fix #81, then I think some unit test should be added for usage with the node: prefix?

There's no need for explicit tests, just import them.

@aladdin-add
Copy link

@thernstig means: good to have a few tests in here:

valid: [
"const fs = require('fs'); fs.createReadStream()",
"const fs = require('fs'); fs.accessSync()",
"const fs = require('fs'); fs.promises.access()",
"const {promises} = require('fs'); promises.access()",
"const {promises: fs} = require('fs'); fs.access()",
"const {promises: {access}} = require('fs'); access()",
"import fs from 'fs'; fs.promises.access()",
"import * as fs from 'fs'; fs.promises.access()",
"import {promises} from 'fs'; promises.access()",
"import {promises as fs} from 'fs'; fs.access()",

to make sure it's working as expected and avoid potential regressions. 😄

@piranna
Copy link
Author

piranna commented Mar 10, 2023

Oh! makes sense :-)

@piranna
Copy link
Author

piranna commented May 9, 2023

Any update on this? master branch already requires Node.js 16, so we can safely include this PR, how does we proceed? I'm very blocked by it...

@aladdin-add
Copy link

it has been landed in aa75610, I'll try to publish it soon.

@piranna
Copy link
Author

piranna commented May 10, 2023

it has been landed in aa75610, I'll try to publish it soon.

Glad to know, thank you.

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

Successfully merging this pull request may close these issues.

Self imports are not working
3 participants