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

fix: avoid using eval to get the corepack version #45

Merged
merged 1 commit into from
Aug 8, 2021

Conversation

merceyz
Copy link
Member

@merceyz merceyz commented Aug 1, 2021

What's the problem this PR addresses?

#42 used eval to get the current corepack version which doesn't work if the path to corepack isn't .../node_modules/corepack

How did you fix it?

Use a statically analysable require

@arcanis
Copy link
Contributor

arcanis commented Aug 1, 2021

Only the reliance on the Node resolution is a problem, the eval should be kept. Otherwise Webpack will bundle the package.json inside the generated file, which is what I wanted to avoid.

@ljharb
Copy link
Member

ljharb commented Aug 1, 2021

Typically that’s avoided with webpack config, rather than altering code to please a bundler.

@aduh95
Copy link
Contributor

aduh95 commented Aug 1, 2021

There are other solutions, such as https://github.com/andrewimm/babel-plugin-inline-package-json.

@arcanis
Copy link
Contributor

arcanis commented Aug 1, 2021

Right, but this one works portably without requiring extra configuration that future readers would have to be aware of. I'm not sure I see a practical issue here (apart from the corepack/package.json string which should just be ../package.json).

@aduh95
Copy link
Contributor

aduh95 commented Aug 1, 2021

Future readers would have to be aware we are using eval to hack around Webpack, this PR suggests a cleaner solution I think. And it works without any extra configuration, or maybe I misunderstood what you meant by that.
If you do add some extra configuration though, it can address your concern of Webpack inlining the whole package.json file (either by embedding only the version string, or by leaving the require() call in the bundle file).

@ljharb
Copy link
Member

ljharb commented Aug 1, 2021

Pretty sure you can also do (0, require) without using eval or configuring webpack.

@arcanis
Copy link
Contributor

arcanis commented Aug 8, 2021

Let's include the package.json then.

Pretty sure you can also do (0, require) without using eval or configuring webpack.

That would generate a warning on Webpack (and an exception on other bundlers), due to them detecting the use of require in a non-static-analyzable way.

@arcanis arcanis merged commit 78d94eb into nodejs:main Aug 8, 2021
@merceyz merceyz deleted the merceyz/fix/version branch August 8, 2021 19:07
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.

4 participants