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

refactor: convert all file paths to file URLs #1278

Closed
FredKSchott opened this issue Oct 11, 2020 · 17 comments
Closed

refactor: convert all file paths to file URLs #1278

FredKSchott opened this issue Oct 11, 2020 · 17 comments

Comments

@FredKSchott
Copy link
Owner

Background

We have a ton of trouble maintaining file paths resolution logic across Windows, Mac, and Linux. Many of us develop on Unix systems, where paths and URLs can be treated similarly. But windows paths are different, and our code can break if we aren't incredibly, INCREDIBLY careful with every line of file path manipulation. Even when we are careful, it still breaks.

Node added support for file: URLs for this exact reason. File URLs are a fully URL-compatible way to reference files on the file system, convertable to-and-from traditional file paths:

Node can even read & write to the file system directly using these file URLS:

Feature Request

Our codebase would be easier to maintain and less error-prone if we replaced every internal use of a file path with a file URL instead. I'm not sure how much work is required, but it would be good to start investigating this.

Note that this should only impact our internal representations to start, and should not impact the plugin ecosystem at all. We can always convert file URL<>path whenever plugins need to work with a file reference.

@juanfrank77
Copy link

Hey Fred, been a fan of the work you're doing with Snowpack and have been using it for most of my personal projects for the last two months. In the spirit of Hacktoberfest I figured this would a great chance to be useful for the project and since you mentioned this issue is relatively straightforward, I think I should start here.

Problem is, I'm more a UI kind of guy so I don't know much about the internals of a project like this but I do have some experience with file paths in both Windows and Linux. So my question would be, what's a proper way to get started tackling this issue (besides reading the nodejs links you posted)
Thanks!

@FredKSchott
Copy link
Owner Author

Thanks! This may take you out of your comfort zone a bit into Node.js world, but the nice thing about this work specifically is that our test suite will guide you most of the way. I definitely think it's worth giving a try, at least to learn more.

I tried looking around for good blog posts on the subject, but I couldn't find any!

@juanfrank77
Copy link

Yeah, Node.js world is a foreign land for me but I like that you mentioned that the test suite is going to tell me about any screwups I do 😅

Appreciate that you looked into other resources, I think it would be a worthy challenge to undertake but it'll be slow progress 'till I pick up some speed with this.

@FredKSchott
Copy link
Owner Author

Np, and no pressure! If theres no activity here in a week or some, im sure someone else can jump in to help

@high1
Copy link
Contributor

high1 commented Oct 19, 2020

I can take this, if help is still needed.

@dmail
Copy link

dmail commented Oct 27, 2020

I ran into this kind of concerns before and made a package dedicated to this:

An extract from https://github.com/jsenv/jsenv-util#presentation

This repository exists mostly to work with files relative to a directory with an approach that works on windows and linux filesystems as shown in the code example below.

This repository makes me win tons of time when writing js code handling files. Let me know if you find it useful :)

@juanfrank77
Copy link

@dmail That repo looks really cool!
Definitely looks like a more dependable solution than what I was working on.

@FredKSchott
Copy link
Owner Author

@high1 if you're interested, we'd love your help!

One thing to note is that this doesn't need to be a FULL CHANGE THE ENTIRE WORLD AT ONCE kind of project. Feel free to take an incremental, small PRs approach to this work.

@high1
Copy link
Contributor

high1 commented Oct 29, 2020

@FredKSchott yes, I am. If you could point me to one starting point inside the project on which I could start work from, it would be great.

@FredKSchott
Copy link
Owner Author

Honestly, the easiest place to start may be FileBuilder in build.ts since it's a self-contained class. filepath, outDir, output, and more all use file paths (/a/b/c.js) and could be updated to file URLs without affecting too much of the rest of the Snowpack codebase.

@high1
Copy link
Contributor

high1 commented Oct 30, 2020

OK, thanks. I tried to test everything on Windows, the tests are passing mostly, I need to install Chrome to be able to run @web/test-runner tests.

@FredKSchott
Copy link
Owner Author

Huh, I thought that @web/test-runner came with a version of headless chromium. There may be some docs here to help you get up and running: https://modern-web.dev/docs/test-runner/overview/

@high1
Copy link
Contributor

high1 commented Oct 30, 2020

I would need to add some config there, already did that for my snowpack configs. I have a preliminary patch for FileBuilder, will make a merge request soon.

@high1
Copy link
Contributor

high1 commented Oct 30, 2020

Oh, by the way, found on this page Test Runner: Browsers:
By default, the test runner will look for a globally installed Chrome on your computer.
I could create a PR to run test on a headless chrome with Puppeteer, if that's OK.

@high1
Copy link
Contributor

high1 commented Nov 6, 2020

@FredKSchott I took a shot at this, and created a PR. I'd like to have some input on the PR, when you can...

@high1
Copy link
Contributor

high1 commented Nov 24, 2020

I was looking at import-resolvers, and I'm not sure what should be done with it in context of this refactor? @drwpow @FredKSchott I could use an opinion with in-depth knowledge.

@FredKSchott
Copy link
Owner Author

I'm less bullish on this now, and am going to propose closing this issue.

File URLs are a great idea (behave and look the same on both Windows and Linux) but you lose access to all of the path helper methods. Node's URL helper methods aren't as easy to use (for example: path.resolve, path.relative, and path.join don't have equivalent one-liners.

Instead, we should look towards replacing our usage of path with https://www.npmjs.com/package/upath which is better supported and meant to be a drop-in replacement for path.

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

No branches or pull requests

4 participants