File builder url refactor

[high1]

Changes

This starts the refactor from string to URLs beginning with FileBuilder.

Testing

Test were ran locally with same results before and after the PR.

Docs

Refactoring only

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pikapkg/snowpack/lg70dgcvf
✅ Preview: https://snowpack-git-file-builder-url-refactor.pikapkg.vercel.app

[drwpow]

Sorry for the delay on reviewing, but what problem does this address? If I missed a discussion or issue # I’m happy to read up about it; I‘m just out of context on the improvement/fix here.

[high1]

Sorry for the delay on reviewing, but what problem does this address? If I missed a discussion or issue # I’m happy to read up about it; I‘m just out of context on the improvement/fix here.

Hey, @drwpow, it's fine. This was actually done as a starting point for this issue, #1278.

[drwpow]

Ah that‘s right! Thanks for reminding me.

[drwpow]

This all looks good to me! Sorry about the merge conflicts; I believe that‘s a product of the slow review. But what‘s here looks great.

[vercel]

@high1 is attempting to deploy a commit to the Pika Team on Vercel.

A member of the Team first needs to authorize it.

[drwpow]

@high1 Thanks! I can merge this whenever you’re ready. Is this good to go?

[high1]

@high1 Thanks! I can merge this whenever you’re ready. Is this good to go?

@drwpow I think it's good to go. If you have any tips on where to continue with this refactor, would be a lot of help.

[drwpow]

If you have any tips on where to continue with this refactor, would be a lot of help.

Yeah, so what Fred outlined in #1278 is really glossing over a lot of weirdness that we‘ve had to do to resolve URLs. This is something that you‘ll want to pay very close attention to the snapshots on, as every . and / matters!

In general, I think I‘d search the snowpack/ TypeScript codebase for things like path.resolve(…) and path.posix.*. You‘ll find a lot of references to these all over the place, but in particular 2 stand out:

• resolveImports() in build.ts#207
• resolveResponseImports() in dev.ts#739

Both are pretty messy code that‘s juggling Unix and Windows paths trying to resolve files. I think in general the #1278 refactor is of unclear scope, and that‘s hard, but if I had to pick a center, I‘d say the 2 functions here are at the root of a number of bugs we‘ve had to fix in the past, probably because we‘re doing “too much“ work there.

In other words, there aren‘t any known issues at the moment, but as you can see, path resolution logic in Snowpack is currently way more verbose and complicated than it probably needs to be, and we‘re trying to just clean house here.