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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Mar 3, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion .gitignore
View file
Open in desktop
Expand Up @@ -4,4 +4,6 @@
/node_modules
/test.js
.eslintcache
.vscode
.vscode
converted-esm/
Copy link

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?

Copy link

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.

Copy link

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?

Copy link

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 :-)

*.tgz