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

Don't attempt to resolve URLs starting with '#' #6504

Merged
merged 1 commit into from
Jul 5, 2021

Conversation

hbilles
Copy link
Contributor

@hbilles hbilles commented Jun 22, 2021

↪️ Pull Request

This resolves the issue I came across in #6471 where Stylus files containing a rule like clip-path: url('#clip-path') caused the build to fail and throw an error (Received URL without a pathname #clip-path).

At first I thought the issue was coming from the @parcel/transformer-stylus package, but after digging into it I discovered the error is being thrown before the stylus transformer is hit.

After looking around I came across this issue with # in paths of SVG files which was solved in #5188. I chose to update PathRequest.js to check if a path starts with # and return null if so.

I can't think of any cases where we would want Parcel to attempt resolving a path starting with #, but I could be wrong.

Let me know if there's a different path I should take or if there's any issues with my proposed fix.

Thanks!

💻 Examples

This is an example of a Stylus rule that causes Parcel to fail and is fixed by this PR:

.svg-background .cls-1
  clip-path: url('#clip-path')

🚨 Test instructions

✔️ 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

@height
Copy link

height bot commented Jun 22, 2021

Link Height tasks by mentioning a task ID in the pull request title or description, commit messages, or comments.

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

) {
// A protocol-relative URL, e.g `url('//example.com/foo.png')`
return null;
if (dep.specifierType === 'url') {
Copy link
Member

Choose a reason for hiding this comment

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

Ideally we would let plugins that wanted to handle urls do so. Maybe this should be in the default resolver plugin instead?

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 agree it would make sense to have a check like this in something like the default resolver plugin. However I can't see another way of bypassing the if (typeof parsed.pathname !== 'string') block around line 184/187 below which throws an error when it attempts to parse a "path" like #clip-path.

I tried handling the case in packages/resolvers/default/src/DefaultResolver.js but nothing seems to work, I suspect because the error is being thrown before any resolver plugins get hit. Part of the reason I chose to modify the code in PathRequest is because it saw it handling a similar case for paths starting with //.

I'm not that familiar with the structure of the Parcel codebase, so if you have any suggestions for a better way to handle the case of a path starting with # I'm happy to try it.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, thanks. Given that this is already the case, this is fine for now.

@devongovett devongovett linked an issue Jun 27, 2021 that may be closed by this pull request
@devongovett devongovett merged commit 96e2024 into parcel-bundler:v2 Jul 5, 2021
lettertwo added a commit that referenced this pull request Jul 13, 2021
* v2: (34 commits)
  Wrap assets recursively when any incoming dependency is wrapped (#6572)
  Improvements for library targets (#6570)
  Diagnostic for undeclared external dependencies in library builds (#6564)
  More bugs (#6567)
  Don't require `url:` for image transformer (#6565)
  Remove 'Name already registered with serializer' error (#6566)
  Fix live bindings and `this` of external CommonJS modules (#6548)
  JS runtime improvements (#6531)
  Make sure the absolute path isn't contained in the cache (#5900)
  Adds '@parcel/diagnostic' to dependencies (#6563)
  Disable workers with string literals and improve diagnostics (#6536)
  Bug fixes (#6541)
  Don't attempt to resolve URLs starting with '#' (#6504)
  Correctly set worker's output format if not support by environment (#6534)
  Babel: Recognize peerDependencies in isJSX (#6497)
  fix setHeaders ordering on dev server (#6500)
  Graph: Remove Node interface (#6530)
  Fix TS build script for old Node versions (#6526)
  Improve library targets (#6517)
  Fix TypeScript and other sourcemaps by always creating an initial sourcemap (#6472)
  ...
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.

Bundler fails when processing CSS clip-path URL
2 participants