-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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 absolute and tilde paths #1324
Fix absolute and tilde paths #1324
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1324 +/- ##
==========================================
- Coverage 88.41% 87.68% -0.73%
==========================================
Files 80 80
Lines 4584 4532 -52
==========================================
- Hits 4053 3974 -79
- Misses 531 558 +27
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGFM. I cannot believe, it completely did not work after #850
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some assets already have their own resolver instance. Could you clean this up a bit and remove those, and reference them to this.resolver instead of creating their own. (As your Pr adds one to the asset instance, they no longer need to create one themselves, they can utilise the one in asset)~
I think the ones with their own resolver instance are SASS, LESS and Stylus
EDIT: On second thought this could probably break on css imports that don't define an extension, what do you think?
Maybe I can create a logic that checks whether the assets already has a resolver or else use the default one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've had a little chat with @fathyb about the possible side-effects of my remark and it's probably a better idea to keep this as is.
So feel free to ignore my previous requested changes.
@marcosbozzani does this work for you now? I'm still getting the same errors on your example repo, and on my own. |
still getting the same errors |
Fix addURLDependency resolution in Asset.js Minimal reproducible repo: https://github.com/marcosbozzani/parcel-bug-tilde-abs-paths Just clone the repo, `yarn install` and `yarn tilde` or `yarn slash` Tilde error: `Cannot resolve dependency './~\style.css'` Slash error: `Cannot resolve dependency './..\..\..\..\..\..\`
Fix addURLDependency resolution in Asset.js Minimal reproducible repo: https://github.com/marcosbozzani/parcel-bug-tilde-abs-paths Just clone the repo, `yarn install` and `yarn tilde` or `yarn slash` Tilde error: `Cannot resolve dependency './~\style.css'` Slash error: `Cannot resolve dependency './..\..\..\..\..\..\`
Fix addURLDependency resolution in Asset.js
Minimal reproducible repo: https://github.com/marcosbozzani/parcel-bug-tilde-abs-paths
Just clone the repo,
yarn install
andyarn tilde
oryarn slash
Tilde error:
Cannot resolve dependency './~\style.css'
Slash error:
Cannot resolve dependency './..\..\..\..\..\..\