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 relative paths in CSS (take 2) #2770

Closed
wants to merge 5 commits into from
Closed

Fix relative paths in CSS (take 2) #2770

wants to merge 5 commits into from

Conversation

devongovett
Copy link
Member

This is take 2 of #2740. It changes addURLDependency to return a relative path, fixes the test to resolve the asset relative to the css file, and adds a test for importing a raw asset in JS in a subdirectory.

Fixes #2736. Closes #2740.

.getAsset(resolved, this.options)
.generateBundleName();

parsed.pathname = path.relative(path.dirname(this.relativeName), name);
Copy link
Member

@mischnic mischnic Mar 11, 2019

Choose a reason for hiding this comment

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

name is always just a hash+extension, so this relative call doesn't really do anything useful.

Here's a repo where it still isn't working (the error resurfaced I tried to fix the original relative PR): https://github.com/mischnic/2736-svg-import
In the dist root, a CSS file contains url('../logo_color.78fe8740.svg');.

(I also think this should be path.posix.relative to not generate backslashes on Windows.)

Copy link
Member Author

Choose a reason for hiding this comment

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

the test for CSS that I updated fails without this line...

Copy link
Member

@mischnic mischnic Mar 11, 2019

Choose a reason for hiding this comment

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

This is the core problem in the repo I linked above:

Relative name of asset: a/style.css. A URL Dependency is in the outputDir root, so ../logo_color.78fe8740.svg seems correct.

Actually, a/style.css gets put into the root as well (does the Asset know this or only the Packager?) and now the ../ part is wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

ahhh you're right. I guess we cannot correctly generate the relative path.

@devongovett devongovett deleted the relative-css2 branch March 12, 2019 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect svg filepath
2 participants