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.

Sign up for GitHub

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

Jump to bottom

Allow to configure the resolve-url-loader #603

Merged
merged 7 commits into from Aug 9, 2019
Merged

Allow to configure the resolve-url-loader #603

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
988e5d1
Update resolve-url-loader to v3.1.0
diegocardoso93 Jul 8, 2019
906ae9b
Fix "found orphan CR, try removeCR option"
diegocardoso93 Jul 8, 2019
01e1da7
Add optional resolve-url-loader options
Jul 17, 2019
a52bb05
Add optional resolve-url-loader options
diegocardoso93 Jul 17, 2019
392d32b
Merge branch 'master' of https://github.com/diegocardoso93/webpack-en…
Jul 17, 2019
4671016
Fix lint job
Jul 26, 2019
b7c7e66
Add documentation for resolveUrlLoaderOptions
Jul 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 8 additions & 1 deletion lib/WebpackConfig.js
View file
Open in desktop
Expand Up @@ -115,7 +115,14 @@ class WebpackConfig {
// Features/Loaders options
this.copyFilesConfigs = [];
this.sassOptions = {
resolveUrlLoader: true
resolveUrlLoader: true,
resolveUrlLoaderOptions: {
engine: 'postcss',
diegocardoso93 marked this conversation as resolved.
Show resolved Hide resolved
keepQuery: false,
removeCR: false,
debug: false,
silent: false
}
};
this.preactOptions = {
preactCompat: false
Expand Down
3 changes: 2 additions & 1 deletion lib/loaders/sass.js
View file
Open in desktop
Expand Up @@ -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:

webpack-encore/.eslintrc.js

Line 9 in 9a3cb43

"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.

}
});
}
Expand Down
20 changes: 20 additions & 0 deletions test/loaders/sass.js
View file
Open in desktop
Expand Up @@ -65,6 +65,26 @@ describe('loaders/sass', () => {
cssLoader.getLoaders.restore();
});

it('getLoaders() with resolve-url-loader options', () => {
const config = createConfig();
config.enableSassLoader(() => {}, {
resolveUrlLoaderOptions: {
removeCR: true
}
});

// make the cssLoader return nothing
sinon.stub(cssLoader, 'getLoaders')
.callsFake(() => []);

const actualLoaders = sassLoader.getLoaders(config);
expect(actualLoaders).to.have.lengthOf(2);
expect(actualLoaders[0].loader).to.equal('resolve-url-loader');
expect(actualLoaders[0].options.removeCR).to.be.true;

cssLoader.getLoaders.restore();
});

it('getLoaders() without resolve-url-loader', () => {
const config = createConfig();
config.enableSassLoader(() => {}, {
Expand Down