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 autoinstall of cssnano #2415

Merged
merged 1 commit into from Dec 17, 2018

Conversation

mischnic
Copy link
Member

@mischnic mischnic commented Dec 14, 2018

↪️ Pull Request

Fixes autoinstalling of cssnano (previously, parcel tried to install the package "cssnano/package.json".
Fixes #2290, this seems to be a cleaner/easier fix than what was proposed there.

With this change, localrequire("cssnano/package.json") never fails, because it will only run after cssnano is installed.

Should there be a test for this? If yes, how should that look like?

💻 Examples

🚨 Test instructions

Building any html file which imports a css file.

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

@@ -54,11 +54,8 @@ async function getConfig(asset) {
}

if (asset.options.minify) {
let [cssnano, {version}] = await Promise.all(
['cssnano', 'cssnano/package.json'].map(name =>
localRequire(name, asset.name).catch(() => require(name))
Copy link
Member Author

@mischnic mischnic Dec 14, 2018

Choose a reason for hiding this comment

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

.catch(() => require(name))
What was that for?

Copy link
Member

Choose a reason for hiding this comment

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

not sure, seems unnecessary. do you remember @DeMoorJasper?

@mischnic
Copy link
Member Author

mischnic commented Dec 15, 2018

Or this should be fixed in localRequire itself? I still think that the code in postcss.js should be changed, because requiring cssnano and the package.json at the same time isn't faster and only more complex.

@devongovett
Copy link
Member

This is a fine fix for now. I do think we should fix the parsing of package names in localRequire as well though. There is a function in the resolver for this that maybe we can pull out/use. Want to make a followup PR for that?

@devongovett devongovett merged commit c74c98b into parcel-bundler:master Dec 17, 2018
@mischnic mischnic deleted the cssnano-autoinstall branch December 17, 2018 06:59
@mischnic
Copy link
Member Author

There is a function in the resolver for this that maybe we can pull out/use. Want to make a followup PR for that?

-> #2425

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.

cssnano Host Key Verification Fails
2 participants