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

Correctly build dependency URLs (for CSS) #2740

Merged
merged 2 commits into from Mar 12, 2019
Merged

Correctly build dependency URLs (for CSS) #2740

merged 2 commits into from Mar 12, 2019

Conversation

mischnic
Copy link
Member

@mischnic mischnic commented Mar 7, 2019

↪️ Pull Request

Fixes #2736
Fix underlying issue and undo workaround of #2518

💻 Examples

Dependency URLs for bundles generated in subfolders would contain something like this <img src="/../logo_color.78fe8740.svg"> (not properly resolved).

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

Fix real core issue of #2518
budparr pushed a commit to theNewDynamic/thenewdynamic.org that referenced this pull request Mar 8, 2019
Parcel parcel-bundler/parcel#2740

parcel-bundler/parcel#2736

also removed --experimental-scope-hoisting until upgrade works
Copy link
Member

@DeMoorJasper DeMoorJasper left a comment

Choose a reason for hiding this comment

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

Tests are failing

@mischnic
Copy link
Member Author

mischnic commented Mar 8, 2019

In some places absolute urls are used (with the publicURL) and in others a relative url. Should the absolute url be used everywhere? (That's why the tests are failing)

@font-face {
  font-family: "Test";
  src: url("/test.80d5c592.woff2") format("woff2");
  /* vs */
  src: url("test.80d5c592.woff2") format("woff2");
}

@DeMoorJasper
Copy link
Member

DeMoorJasper commented Mar 8, 2019

@mischnic I think they should both be possible, not sure what the behaviour should be though.
I think absolute paths should work like they do everywhere in Parcel. (same for ~node_module and ~/ paths, although those are currently not supported, and not sure if we should even support that)

@devongovett
Copy link
Member

Why do we need to switch to absolute paths for CSS? One of the nice things about using relative paths is that you can move the base directory around where you deploy and it still works.

@mischnic
Copy link
Member Author

Why do we need to switch to absolute paths for CSS? One of the nice things about using relative paths is that you can move the base directory around where you deploy and it still works.

With the current master, there are some cases where the paths somehow don't work: e.g. #2758 and #2736

Apart from that, sometimes Parcel uses a relative (this case) and sometimes a absolute path (with publicURL, e.g. in html url dependencies)

@devongovett
Copy link
Member

I think we can probably still use the relative path for CSS, but it needs to be set here instead of in replaceBundleNames:

parsed.pathname = this.options.parser
.getAsset(resolved, this.options)
.generateBundleName();

@devongovett
Copy link
Member

Probably should also add a test for #2736 as well.

@rhurstdialpad
Copy link
Contributor

When can we get a patch release on this? We need this ASAP at Dialpad.

@mischnic
Copy link
Member Author

When can we get a patch release on this? We need this ASAP at Dialpad.

This is already released as 1.12.1.

@rhurstdialpad
Copy link
Contributor

@mischnic @devongovett @DeMoorJasper Excellent, thanks guys. If you're ever in Vancouver, beers on me 🍻

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
4 participants