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

fix(build): override publicPath for ExtractTextPlugin #4036

Merged
merged 1 commit into from
Jan 17, 2017
Merged

fix(build): override publicPath for ExtractTextPlugin #4036

merged 1 commit into from
Jan 17, 2017

Conversation

changLiuUNSW
Copy link
Contributor

@changLiuUNSW changLiuUNSW commented Jan 16, 2017 •
edited
Loading

All our CSS assets are in a flat structure in the dist folder, therefore all relative URLs are simply ./[asset name]
Unfortunately, if we add publicPath via deployUrl, ExtractTextPlugin picks up the output.publicPath and rewrites assets url path with [publicPath]/[asset name]
fixes #4035

@filipesilva
Copy link
Contributor

@changLiuUNSW can you add a test for this case? I want to ensure we don't break it in the future.

A good test would be new file extract-css.ts in https://github.com/angular/angular-cli/tree/master/tests/e2e/tests/build/styles, with a test for ng build and ng build --deploy-url, confirming your case (via background: url("./assets/svg/more.svg"); etc).

@filipesilva filipesilva self-assigned this Jan 16, 2017
@changLiuUNSW changLiuUNSW changed the title feat(build): override publicPath for ExtractTextPlugin fix(build): override publicPath for ExtractTextPlugin Jan 16, 2017
@changLiuUNSW
Copy link
Contributor Author

changLiuUNSW commented Jan 16, 2017 •
edited
Loading

@filipesilva
Test added.

filipesilva
filipesilva suggested changes Jan 16, 2017
View reviewed changes
Copy link
Contributor

@filipesilva filipesilva left a comment

Choose a reason for hiding this comment

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

There's a change I'm requesting regarding correctness that is important (checking the generated path), the others are minor changes.

tests/e2e/tests/build/styles/extract-css.ts
// build with deloyUrl
.then(() => ng('build', '--deploy-url=client/'))
.then(() => expectFileToMatch('dist/styles.bundle.css',
/div\s*{\s*background:\s*url\(more.svg\);\s*}/))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you confirm the path here? If I get the issue correctly, this test would pass even without your fixe since it's just checks that there is an url that has more.svg.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without my fix, when we add --deploy-url=client/, the result is:

div { background: url(client/more.svg); }

Test will fail in this case.

tests/e2e/tests/build/styles/extract-css.ts
.then(() => ng('build'))
.then(() => expectFileToMatch('dist/styles.bundle.css',
/div\s*{\s*background:\s*url\(more.svg\);\s*}/))
.then(() => ng('build', '--prod'))
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to test the prod builds, it shouldn't change anything regarding --extract-css.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

tests/e2e/tests/build/styles/extract-css.ts
.then(() => ng('build', '--deploy-url=client/'))
.then(() => expectFileToMatch('dist/styles.bundle.css',
/div\s*{\s*background:\s*url\(more.svg\);\s*}/))
.then(() => ng('build', '--prod', '--deploy-url=client/'))
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for prod test, see above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

tests/e2e/tests/build/styles/extract-css.ts
.then((stylesBundle) => expectFileToMatch(stylesBundle,
/div\s*{\s*background:\s*url\(more\..*svg\)\s*}/))
// build with deloyUrl
.then(() => ng('build', '--deploy-url=client/'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you force --extract-css here just to be more explicit to the next person editing this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@filipesilva filipesilva merged commit c1f1e0c into angular:master Jan 17, 2017
@filipesilva
Copy link
Contributor

Thanks for this fix @changLiuUNSW!

@changLiuUNSW changLiuUNSW mentioned this pull request Jan 17, 2017
Closed
@changLiuUNSW changLiuUNSW deleted the fixCssAssetPath branch January 17, 2017 13:31
@korve
Copy link

korve commented Jan 28, 2017

I'm having an issue which relates to this issue I think (paths replaced). The project was initialised with a a previous version of angular-cli

Error: No errors
at validate (node_modules/extract-text-webpack-plugin/schema/validator.js:10:9)
at Function.ExtractTextPlugin.extract (node_modules/extract-text-webpack-plugin/index.js:188:3)
at node_modules/angular-cli/models/webpack-build-styles.js:79:83
at Array.map (native)
at Object.getWebpackStylesConfig (node_modules/angular-cli/models/webpack-build-styles.js:76:43)
at new NgCliWebpackConfig (node_modules/angular-cli/models/webpack-config.js:44:51)
at Class.run (node_modules/angular-cli/tasks/build-webpack.js:18:22)
at Class.buildRun (node_modules/angular-cli/commands/build.run.js:39:22)
at Class.run (node_modules/angular-cli/commands/build.js:38:47)
at Class. (node_modules/angular-cli/ember-cli/lib/models/command.js:147:17)
at process._tickCallback (internal/process/next_tick.js:103:7)

@intellix
Copy link
Contributor

@korve please see #4264
Temporary workaround: npm install extract-text-webpack-plugin@2.0.0-rc.0 --save-dev

@korve
Copy link

korve commented Jan 28, 2017

Thank you @intellix!

MRHarrison pushed a commit to MRHarrison/angular-cli that referenced this pull request Feb 9, 2017
@changLiuUNSW @MRHarrison
fix(build): override publicPath for ExtractTextPlugin and add extract…
1c5e88f 
…-css test (angular#4036)
@changLiuUNSW changLiuUNSW mentioned this pull request Apr 19, 2017
Closed
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@filipesilva filipesilva filipesilva approved these changes

Assignees

@filipesilva filipesilva

Labels
state: WIP
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

(publicPath) Problem with CSS url path when I add publicPath(deployUrl) in angular-cli.json
5 participants
@changLiuUNSW @filipesilva @korve @intellix @googlebot