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: safe checking if params are present for at rule #871

Merged
merged 3 commits into from Dec 14, 2018

Conversation

Antonio-Laguna
Copy link
Contributor

@Antonio-Laguna Antonio-Laguna commented Dec 14, 2018

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Tried today to use https://github.com/jonathantneal/postcss-font-magician to autoload Google Fonts and suddenly it was all broken with this error on the CSS:

throw new Error("Module build failed (from ../node_modules/extract-css-chunks-webpack-plugin/dist/loader.js):
ModuleBuildError: Module build failed (from ../node_modules/css-loader/dist/cjs.js):
TypeError: Cannot read property 'toString' of undefined
at css.walkAtRules.atrule (./node_modules/css-loader/dist/plugins/postcss-icss-parser.js:82:58)
at ./node_modules/postcss/lib/container.js:295:18
at ./node_modules/postcss/lib/container.js:135:18
at Root.each (./node_modules/postcss/lib/container.js:101:16)
at Root.walk (./node_modules/postcss/lib/container.js:131:17)
at Root.walkAtRules (./node_modules/postcss/lib/container.js:293:19)
at process (./node_modules/css-loader/dist/plugins/postcss-icss-parser.js:80:7)
at LazyResult.run (./node_modules/postcss/lib/lazy-result.js:295:14)
at LazyResult.asyncTick (./node_modules/postcss/lib/lazy-result.js:208:26)
at LazyResult.asyncTick (./node_modules/postcss/lib/lazy-result.js:221:14)
at LazyResult.asyncTick (./node_modules/postcss/lib/lazy-result.js:221:14)
at ./node_modules/postcss/lib/lazy-result.js:250:14
at new Promise (<anonymous>)
at LazyResult.async (./node_modules/postcss/lib/lazy-result.js:246:23)
at LazyResult.then (./node_modules/postcss/lib/lazy-result.js:127:17)
at Object.loader (./node_modules/css-loader/dist/index.js:122:6)
at runLoaders (./node_modules/webpack/lib/NormalModule.js:301:20)
at ./node_modules/loader-runner/lib/LoaderRunner.js:364:11
at ./node_modules/loader-runner/lib/LoaderRunner.js:230:18
at context.callback (./node_modules/loader-runner/lib/LoaderRunner.js:111:13)
at process.then.catch.error (./node_modules/css-loader/dist/index.js:263:5)");

Digging a bit deeper I found that they use the atRule API by PostCSS as seen here: https://github.com/jonathantneal/postcss-font-magician/blob/master/index.js#L152 Double checked the API https://api.postcss.org/postcss.html#.atRule and it doesn't seem that the params parameter is something that's needed.

I've double checked that all tests are still passing. Wasn't sure where to add a test for this little thing. Happy to add it wherever you may find it useful.

@jsf-clabot
Copy link

jsf-clabot commented Dec 14, 2018

CLA assistant check
All committers have signed the CLA.

@Antonio-Laguna
Copy link
Contributor Author

FWIW I've run again npm run ci:lint:commits locally and it doesn't seem to moan here. Anything I have to change?

@alexander-akait
Copy link
Member

@Antonio-Laguna Just add comment about this exception in code.

FWIW I've run again npm run ci:lint:commits locally and it doesn't seem to moan here. Anything I have to change?

Because it is script for CI. Use fix: safe checking if params are present for at-rule

@alexander-akait
Copy link
Member

Anyway good work, thanks!

To be next to where the infraction happens
@Antonio-Laguna
Copy link
Contributor Author

@evilebottnawi I've caught an error with npm run ci:lint and I've updated the PR accordingly. circleci is still unhappy about it. How do I proceed?

@Antonio-Laguna Antonio-Laguna changed the title Safe checking if params are present fix: safe checking if params are present Dec 14, 2018
@Antonio-Laguna Antonio-Laguna changed the title fix: safe checking if params are present fix: safe checking if params are present for at rule Dec 14, 2018
@alexander-akait
Copy link
Member

alexander-akait commented Dec 14, 2018

@Antonio-Laguna just add comment about check what some plugins remove params and we need check this

@Antonio-Laguna
Copy link
Contributor Author

Not sure I'm understanding you correctly. Where do I add a comment?

@alexander-akait
Copy link
Member

@Antonio-Laguna

// Due reusing `ast` from `postcss-loader` some plugins can remove `params` property, we need check this situation
 if (atrule.params) {
          // eslint-disable-next-line no-param-reassign
          atrule.params = replaceImportsInString(atrule.params.toString());
        }

@Antonio-Laguna
Copy link
Contributor Author

Ahh gotcha!

@Antonio-Laguna
Copy link
Contributor Author

Seems that circle ci is still unhappy :/

@alexander-akait
Copy link
Member

@Antonio-Laguna don't worry about CI

@alexander-akait alexander-akait merged commit a88fed1 into webpack-contrib:master Dec 14, 2018
@Antonio-Laguna
Copy link
Contributor Author

Thanks!

@alexander-akait
Copy link
Member

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.

None yet

3 participants