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

Allow to configure the resolve-url-loader #603

Merged
merged 7 commits into from Aug 9, 2019

Conversation

diegocardoso93
Copy link
Contributor

@diegocardoso93 diegocardoso93 commented Jul 8, 2019

This PR solve the error below, occuring on some cases:

Error: resolve-url-loader: CSS error
  source-map information is not available at url() declaration (found orphan CR, try removeCR option)

See discussion: bholloway/resolve-url-loader#107

Error: resolve-url-loader: CSS error
  source-map information is not available at url() declaration (found orphan CR, try removeCR option)

see: bholloway/resolve-url-loader#107
@diegocardoso93 diegocardoso93 changed the title Update resolve-url-loader to fix fix "found orphan CR, try removeCR option" Update resolve-url-loader to fix "found orphan CR, try removeCR option" Jul 8, 2019
@Lyrkan
Copy link
Collaborator

Lyrkan commented Jul 9, 2019

Hey @diegocardoso93

resolve-url-loader@3.1.0 was already allowed by the current version range (^3.0.1) so the question is: is removeCR: true a good default value?

Do you know if it could have any drawbacks on setups that don't need it to be true (ie. why it isn't already true by default in the resolve-url-loader)?

If that's the case it could be better to allow setting it easily (maybe using a new option in enableSassLoader) and detect the error (using friendly-errors-webpack-plugin) to display a comprehensive message. What do you think?

@diegocardoso93
Copy link
Contributor Author

diegocardoso93 commented Jul 9, 2019

Hello @Lyrkan
Totally agree, it can be better if the decision of enable removeCR option can be take by user.
Could you do this change, please?
Thanks!

@diegocardoso93
Copy link
Contributor Author

@Lyrkan I've just updated with the resolveUrlLoaderOptions.
I don't sure if the option variable name is the best.
Can you take a look?

Copy link
Collaborator

@Lyrkan Lyrkan left a comment

Choose a reason for hiding this comment

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

Hi @diegocardoso93,

Yes, I think that's better, I only have a few remarks :)

Also, could you add some documentation here?

webpack-encore/index.js

Lines 749 to 756 in 9a3cb43

* Supported options:
* * {boolean} resolveUrlLoader (default=true)
* Whether or not to use the resolve-url-loader.
* Setting to false can increase performance in some
* cases, especially when using bootstrap_sass. But,
* when disabled, all url()'s are resolved relative
* to the original entry file... not whatever file
* the url() appears in.

lib/WebpackConfig.js Outdated Show resolved Hide resolved
@@ -31,7 +31,8 @@ module.exports = {
sassLoaders.push({
loader: 'resolve-url-loader',
options: {
sourceMap: webpackConfig.useSourceMaps
sourceMap: webpackConfig.useSourceMaps,
...webpackConfig.sassOptions.resolveUrlLoaderOptions
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think that the spread operator is allowed in this context with the version of ESLint we are currently using (which is why the "lint" job fails on Travis).

So two choices there:

  • update ESLint (but maybe that's a chore for another PR)
  • use Object.assign()

Copy link
Member

Choose a reason for hiding this comment

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

the issue is not the eslint version, but the node.js version.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The linter is executed using Node 10 which should supports this.
It should even be ok under Node 8 since that job is green: https://travis-ci.org/symfony/webpack-encore/jobs/560124277

$ nvm use v8.15.1
Now using node v8.15.1 (npm v6.4.1)

$ node
> const foo = { foo: 1, bar: 2 };
undefined
> const bar = { ...foo, bar: 3 };
undefined
> bar
{ foo: 1, bar: 3 }

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe that could be solved without updating eslint though, by changing the following option of our .eslintrc.js to 2018:

"parserOptions": { "ecmaVersion": 2017 },

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually you were partially right @stof, using ecmaVersion: 2018 is triggering other errors:

  99:22  error  Spread properties are not supported yet on Node 8.0.0                                                           node/no-unsupported-features
  99:22  error  Rest/spread properties are not supported until Node.js 8.3.0. The configured version range is '8.* || >= 10.*'  node/no-unsupported-features/es-syntax

We should probably change that range since most Webpack loaders are now requiring >= 8.9.0 (for instance: css-loader, url-loader, file-loader, ...)

But as I said earlier, maybe it should be handled in another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just updated the PR with changes requested.
Used Object.assign() option for now.

@Lyrkan Lyrkan changed the title Update resolve-url-loader to fix "found orphan CR, try removeCR option" Allow to configure the resolve-url-loader Jul 22, 2019
@weaverryan
Copy link
Member

Very sensible & clean addition. Thank you Diego!

@weaverryan weaverryan merged commit b7c7e66 into symfony:master Aug 9, 2019
weaverryan added a commit that referenced this pull request Aug 9, 2019
…3, Diego)

This PR was merged into the master branch.

Discussion
----------

Allow to configure the resolve-url-loader

This PR solve the error below, occuring on some cases:
```
Error: resolve-url-loader: CSS error
  source-map information is not available at url() declaration (found orphan CR, try removeCR option)
```
See discussion: bholloway/resolve-url-loader#107

Commits
-------

b7c7e66 Add documentation for resolveUrlLoaderOptions
4671016 Fix lint job
392d32b Merge branch 'master' of https://github.com/diegocardoso93/webpack-encore
a52bb05 Add optional resolve-url-loader options
01e1da7 Add optional resolve-url-loader options
906ae9b Fix "found orphan CR, try removeCR option"
988e5d1 Update resolve-url-loader to v3.1.0
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

4 participants