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

URL query params no longer parsed correctly with URL path in version 2.0.0+ #883

Closed
rishitank opened this issue Jan 4, 2019 · 25 comments
Closed

Comments

@rishitank
Copy link

rishitank commented Jan 4, 2019

  • Operating System: Mac OS X 10.14.2
  • Node Version: 10.10.0
  • NPM Version: 6.5.0
  • webpack Version: 4.26.1
  • css-loader Version: 2.1.0

Expected Behavior

When importing a library such as Bootstrap-sass or Font-awesome using SASS, these modules use query parameters in the URLs in order to extract specific styles. For example if I look in node_modules/font-awesome/css/font-awesome.css I see something like this:

@font-face {
  font-family: 'FontAwesome';
  src: url('../fonts/fontawesome-webfont.eot?v=4.7.0');
  src: url('../fonts/fontawesome-webfont.eot?#iefix&v=4.7.0') format('embedded-opentype'), url('../fonts/fontawesome-webfont.woff2?v=4.7.0') format('woff2'), url('../fonts/fontawesome-webfont.woff?v=4.7.0') format('woff'), url('../fonts/fontawesome-webfont.ttf?v=4.7.0') format('truetype'), url('../fonts/fontawesome-webfont.svg?v=4.7.0#fontawesomeregular') format('svg');
  font-weight: normal;
  font-style: normal;
}

Actual Behavior

However when simply importing this module and setting the correct fa-font-path, it seems that css-loader is processing the query parameters incorrectly:

ERROR in ./src/containers/widgets/Calendar/assets/react/Calendar.scss
Module build failed (from ./node_modules/mini-css-extract-plugin/dist/loader.js):
ModuleParseError: Module parse failed: Unterminated string constant (5:134)
You may need an appropriate loader to handle this file type.
| var urlEscape = require("../../../../../../node_modules/css-loader/dist/runtime/url-escape.js");
| var ___CSS_LOADER_URL___0___ = urlEscape(require("bootstrap-sass/assets/fonts/bootstrap/glyphicons-halflings-regular.eot\""));
> var ___CSS_LOADER_URL___1___ = urlEscape(require("bootstrap-sass/assets/fonts/bootstrap/glyphicons-halflings-regular.eot") + "?#iefix"");
| var ___CSS_LOADER_URL___2___ = urlEscape(require("bootstrap-sass/assets/fonts/bootstrap/glyphicons-halflings-regular.woff2\""));
| var ___CSS_LOADER_URL___3___ = urlEscape(require("bootstrap-sass/assets/fonts/bootstrap/glyphicons-halflings-regular.woff\""));
    at handleParseError (/Users/matchbyte/Documents/git/Matchbyte/Tigerair-WhiteKnight-Modules/node_modules/webpack/lib/NormalModule.js:447:19)
    at doBuild.err (/Users/matchbyte/Documents/git/Matchbyte/Tigerair-WhiteKnight-Modules/node_modules/webpack/lib/NormalModule.js:481:5)
    at runLoaders (/Users/matchbyte/Documents/git/Matchbyte/Tigerair-WhiteKnight-Modules/node_modules/webpack/lib/NormalModule.js:342:12)
    at /Users/matchbyte/Documents/git/Matchbyte/Tigerair-WhiteKnight-Modules/node_modules/loader-runner/lib/LoaderRunner.js:370:3
    at iterateNormalLoaders (/Users/matchbyte/Documents/git/Matchbyte/Tigerair-WhiteKnight-Modules/node_modules/loader-runner/lib/LoaderRunner.js:211:10)
    at iterateNormalLoaders (/Users/matchbyte/Documents/git/Matchbyte/Tigerair-WhiteKnight-Modules/node_modules/loader-runner/lib/LoaderRunner.js:218:10)
    at /Users/matchbyte/Documents/git/Matchbyte/Tigerair-WhiteKnight-Modules/node_modules/loader-runner/lib/LoaderRunner.js:233:3
    at context.callback (/Users/matchbyte/Documents/git/Matchbyte/Tigerair-WhiteKnight-Modules/node_modules/loader-runner/lib/LoaderRunner.js:111:13)
    at process.then.result (/Users/matchbyte/Documents/git/Matchbyte/Tigerair-WhiteKnight-Modules/node_modules/css-loader/dist/index.js:262:12)
 @ ./src/containers/widgets/Calendar/view/react/Calendar.jsx 38:0-42
 @ ./src/containers/widgets/Calendar/Calendar.react.jsx
 @ ./src/containers/containersBundle.react.js
 @ ./src/index.js
 @ multi ./node_modules/react-dev-utils/webpackHotDevClient.js ./config/polyfills.js ./src/index.js

Furthermore, it appears that in some cases the string passed to require terminates with \"" instead of "

@rishitank rishitank changed the title URL query params no longer joined correctly with URL path in version 2.0.0+ URL query params no longer parsed correctly with URL path in version 2.0.0+ Jan 4, 2019
@alexander-akait
Copy link
Member

Looks like a bug, need fix

@alexander-akait
Copy link
Member

@rishitank Can't reproduce, please create minimum reproducible test repo

@alexander-akait
Copy link
Member

Looks you have loader what break url

@rishitank
Copy link
Author

Yes after updating css loader to 2.1.0 the above error occurs. Reverting to version 1.0.1 the error does not occur. I've isolated the issue to css loader, the error is caused by bad output from css loader as mentioned in the error message.

@alexander-akait
Copy link
Member

alexander-akait commented Jan 9, 2019

@rishitank Can't reproduce your problem, please create minimum reproducible test repo

I can send a PR with tests and provide link on CI and you can see what is works as expected, so i can't help you without minimum reproducible test repo

@rishitank
Copy link
Author

@evilebottnawi you may try this test repo: https://github.com/rishitank/test

If you try running npm start it will error as above. If I were to downgrade css-loader to version 1.0.1, and update src/index.scss by removing the alias ~ prefix in $icon-font-path and $fa-font-path then it runs successfully.

@alexander-akait
Copy link
Member

@rishitank why you use ~ https://github.com/rishitank/test/blob/master/src/index.scss#L2 for variables? ~ require only for module resolution and should be not using for variables

@rishitank
Copy link
Author

@evilebottnawi If I try without ~, then css-loader will resolve the URL as a relative directory instead of from node_modules.

@rishitank
Copy link
Author

@evilebottnawi it works without ~ in older versions of css-loader, but in the new version it seems this is needed to avoid working with relative directories.

@alexander-akait
Copy link
Member

@rishitank for relative directory use ./ and ../, you don't need module resolution here

@rishitank
Copy link
Author

@evilebottnawi module resolution is needed to determine the icon-font-path and fa-font-path, previously this was working without any adjustment and it would resolve font-awesome and bootstrap-sass in index.scss but now it is unable to find these paths due to changes in the URL parsing of css-loader.

@alexander-akait
Copy link
Member

@rishitank icon-font-path and fa-font-path are variables, it is not import/require or other module loader system, why you need override defaults paths?

@rishitank
Copy link
Author

@evilebottnawi after some configuration changes, I'm able to get this working now. It seems $bootstrap-sass-asset-helper: true; is no longer needed when combined with the changes in css-loader 2.0.0+

@alexander-akait
Copy link
Member

@rishitank 👍 Can we close issue?

@rishitank
Copy link
Author

@evilebottnawi yes

@wmarques
Copy link

wmarques commented Mar 11, 2019

Sorry to comment on a closed issue, but I'm having the exact same problem and removing $bootstrap-sass-asset-helper: true; didn't resolved it.

@evilebottnawi those variables are made to send the path to the icons as parameter so the lib (bootstrap or font-awesome) can have the correct path for the fonts.

Do you have any clue on how to solve this?

Here is the full stacktrace:

ERROR in ./src/app/css/main.scss (./node_modules/css-loader/dist/cjs.js??ref--13-1!./node_modules/sass-loader/lib/loader.js!./src/app/css/main.scss) 16:120
Module parse failed: Unterminated string constant (16:120)
You may need an appropriate loader to handle this file type.
| var ___CSS_LOADER_URL___5___ = urlEscape(require("../fonts/fontawesome-webfont.svg?v=4.7.0") + "#fontawesomeregular");
| var ___CSS_LOADER_URL___6___ = urlEscape(require("./font-path(\"bootstrap/glyphicons-halflings-regular.eot\""));
> var ___CSS_LOADER_URL___7___ = urlEscape(require("./font-path(\"bootstrap/glyphicons-halflings-regular.eot") + "?#iefix"");
| var ___CSS_LOADER_URL___8___ = urlEscape(require("./font-path(\"bootstrap/glyphicons-halflings-regular.woff2\""));
| var ___CSS_LOADER_URL___9___ = urlEscape(require("./font-path(\"bootstrap/glyphicons-halflings-regular.woff\""));

If you prefer I can open a new ticket with those informations.
It seems to fail on the iefix query param...

Thanks !

@alexander-akait
Copy link
Member

@wmarques looks you use old version of css-loader, can you update?

@alexander-akait
Copy link
Member

alexander-akait commented Mar 11, 2019

Something wrong in url(), based on error you have css like:

@font-face {
  /* Other code */
  src: url("./font-path(\"bootstrap/glyphicons-halflings-regular.eot\"")
}

Bootstrap should have variable for assets path, maybe you have invalid value of variable

@wmarques
Copy link

@evilebottnawi Thanks for your fast answer !

I'm at 2.1.1 which seems to be the latest version. Btw it works on the 1.0.1 version.
Just realised that my variable wasn't taken because I set it after, now I set it before and it produces same output but with different path:

| var ___CSS_LOADER_URL___5___ = urlEscape(require("font-awesome/fonts/fontawesome-webfont.svg?v=4.7.0") + "#fontawesomeregular");
| var ___CSS_LOADER_URL___6___ = urlEscape(require("bootstrap-sass/assets/fonts/bootstrapglyphicons-halflings-regular.eot\""));
> var ___CSS_LOADER_URL___7___ = urlEscape(require("bootstrap-sass/assets/fonts/bootstrapglyphicons-halflings-regular.eot") + "?#iefix"");
| var ___CSS_LOADER_URL___8___ = urlEscape(require("bootstrap-sass/assets/fonts/bootstrapglyphicons-halflings-regular.woff2\""));
| var ___CSS_LOADER_URL___9___ = urlEscape(require("bootstrap-sass/assets/fonts/bootstrapglyphicons-halflings-regular.woff\""));

It's strange, my variable is : $icon-font-path: '~bootstrap-sass/assets/fonts/bootstrap/';

I have other variables for the same usage:

$fa-font-path: '~font-awesome/fonts';
$icons-path: '~ag-grid-community/src/styles/ag-theme-balham/icons/';

Those one works but the bootstrap one is failing, maybe because bootstrap requires the font with the additional iefix query param...

@alexander-akait
Copy link
Member

@wmarques can you create minimum reproducible test repo? Something wrong with url content, we need debug this, thanks!

@wmarques
Copy link

@evilebottnawi Do you have any "sample" repo that I can clone ? With a basic webpack config and SASS

@alexander-akait
Copy link
Member

@wmarques just clone any boilerplate, or you can provide link on own repo where you faced with issue (just remove all security data if you have their in repo)

@wmarques
Copy link

wmarques commented Mar 14, 2019

@evilebottnawi sorry for the time, here it is: https://github.com/wmarques/css-loader-bug
Reproducing with both npm start and npm run build scripts

@alexander-akait
Copy link
Member

@wmarques please open new issue, thanks

@wmarques
Copy link

@evilebottnawi done :)

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

No branches or pull requests

3 participants