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

[RFC] Fix for absolute paths #290

Closed
wants to merge 22 commits into from
Closed

[RFC] Fix for absolute paths #290

wants to merge 22 commits into from

Conversation

DeMoorJasper
Copy link
Member

@DeMoorJasper DeMoorJasper commented Dec 15, 2017

Fixes absolute paths in html and other assets closes #86
Removes the need for other project relative solutions closes #336 closes #466

  • Is able to detect and resolve absolute paths, unix, node_modules and project relative
  • Supports both ~ and /

@brandon93s
Copy link
Contributor

Tested on Windows 10. Still seeing the same error message with these changes.

@DeMoorJasper
Copy link
Member Author

Ow it fixed it on os x, i'll look into a better fix on windows in a bit

@DeMoorJasper
Copy link
Member Author

@brandon93s just loaded it up on Windows 10 and this seems resolved, could u elaborate what u exactly tried?

@brandon93s
Copy link
Contributor

index.html:

<!DOCTYPE html>
<html>
<body>
    <h1>Index</h1>        
    <a href="/other.html">Other</h1>
</body>
</html>

other.html

<!DOCTYPE html>
<html>
<body>
    <h1>Other</h1>
</body>
</html>

command:

parcel .\index.html

Same error if I move other.html into a src directory and update the href.

@DeMoorJasper
Copy link
Member Author

DeMoorJasper commented Dec 16, 2017

@brandon93s i can't seem to get an issue with that exact setup i however use
parcel ./index.html --no-cache

@brandon93s
Copy link
Contributor

User error. Checked out the branch again... working...

src/Asset.js Outdated
@@ -65,6 +65,10 @@ class Asset {
return url;
}

if (url.indexOf('/') === 0) {
url = path.join(this.options.rootDir, url);
Copy link
Member

Choose a reason for hiding this comment

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

This will work for files that are in the direct project being bundled, but what about absolute paths inside packages in node_modules? Seems like those should probably be relative to that package, not the whole project maybe?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ow didn't know that happened i'll try find a better way than

@brandon93s brandon93s changed the title Fix for absolute paths [WIP] Fix for absolute paths Dec 22, 2017
@DeMoorJasper DeMoorJasper changed the title [WIP] Fix for absolute paths Fix for absolute paths Jan 1, 2018
@DeMoorJasper DeMoorJasper changed the title Fix for absolute paths [WIP/Needs review/testing] Fix for absolute paths Jan 2, 2018
@DeMoorJasper DeMoorJasper changed the title [WIP/Needs review/testing] Fix for absolute paths Fix for absolute paths Jan 2, 2018
@DeMoorJasper DeMoorJasper changed the title Fix for absolute paths [RFC] Fix for absolute paths Jan 3, 2018
@DeMoorJasper
Copy link
Member Author

DeMoorJasper commented Jan 4, 2018

I improved this PR to support both ~ and / as well as proper detection for unix like absolute paths, added some tests and also support nested node_modules, this now supports any use case i can think of

@guilhermedecampo
Copy link

This is really important!

@davidnagli
Copy link
Contributor

What’s the status of this PR?

@DeMoorJasper
Copy link
Member Author

DeMoorJasper commented Jan 27, 2018

@davidnagli waiting on approval of the community than some finishing up and improving based on new additions to the codebase and than it should be ready to go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants