Fix casing of module locations to match that used by Rollup.

[pkaminski]

Changes

Snowpack uses require.resolve to resolve the absolute location of installed modules. On case-insensitive filesystems, require.resolve will (intentionally) use the cwd case in its result. However, Rollup uses the canonical case for module IDs internally, so if it arrives at an entry point module via a different dependency path it will see it as a "new" module and duplicate it in the installation output.

This PR ensures that Snowpack canonicalizes the case of the module locations before passing them to Rollup. Note that fs.realpathSync.native is needed, as fs.realpathSync explicitly doesn't canonicalize casing. This make the method dependent on the libc that Node was linked against, with two implementation-dependent limitations called out in the docs: the number of symlinks followed may be limited, and, if linked against musl libc, the procfs file system must be mounted on /proc. Neither of these seems like a big issue to me.

Testing

I tested this manually on my Windows machine but I'm not entirely sure how to set up a regression test for it. AFAICT, all the tests are run in both Linux (case-sensitive) and Windows (case-insensitive) environments, and any test for this issue would only work in the latter and break in the former.

---

This change is 

[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/84kkseumf
✅ Preview: Canceled

[update for 2a85070 canceled]

[FredKSchott]

Thanks for filing! Before we go down this route, is there anything that we can do on our end to sanitize the input before we send it to Rollup? Or, is this really a bug in Rollup (that it has this case-sensitivity issue)?

I'm fine to pursue the path you've given here, but would have a slight preference for avoiding adding new limitations/complexity when this is an issue that affects a smaller number of users.

Also if you can, I'd love a few more details on how you've tested the fix. It sounds like this is an issue that you were originally seeing on your case-sensitive Windows machine, but are you saying that our Windows CI environment isn't case-sensitive?

[pkaminski]

It's hard to say where exactly the bug is but I think Snowpack is the only place where we're likely to be able to effect a fix. Rollup needs to treat module paths as case-sensitive since it might be on a case-sensitive file system where "foo/bar.js" and "Foo/bar.js" are in fact different modules. Node has, for whatever reason, decided to use the current cwd case as authoritative throughout its own libraries; this doesn't make sense to me but I assume they've got their reasons and, given that this was an intentional decision a while back, we're unlikely to convince them otherwise. Rollup is using canonically cased paths for keying its internal maps -- I suspect they either use their own file system traversal libraries, or fix up the results obtained from Node's libraries. I doubt it would be productive to try to convince them to use cwd-flavored casing, not least because it's easy to canonicalize a path, but hard to "cwd-canonicalize" one.

Hence, it's up to Snowpack to make sure that any paths we feed into Rollup have canonical casing, no matter how they were obtained. The changes I propose here are one way to do this, by fixing the paths at the "source". Another approach would be to canonicalize the paths as we build Rollup's option objects. The latter is cleaner, in that we only touch one spot in the code. I went with the former as it may prove more robust going forward should the module paths make their way into Rollup through some new code flow (e.g., through a newly added plugin or some such). I'm happy to go with either one, though. (Or both, for good measure!)

I'm not sure why you say that this would affect a small number of users -- anyone on a case-insensitive file system is potentially affected, which means all Windows-based developers and most Mac-based ones I believe (though perhaps Mac shells forcefully canonicalize cwd?). The solution also doesn't introduce any limitations or external complexity, and fairly minimal internal complexity.

I observed the issue because my repository was checked out into C:\Code\workspace... but my shell's cwd was C:\code\workspace\.... On Windows, these are effectively equal paths, but Rollup doesn't know that and was treating modules on the first path (as deduced via transitive dependencies) as different from those on the latter path (as provided by Snowpack's entry points). To reproduce this in a regression test you'd need to cd to a test directory while using the "wrong" case; obviously this would only work on a Windows platform (or Mac, but you don't appear to test on those), since on Linux the badly cased directory wouldn't be found. I'm not sufficiently familiar with Snowpack's test framework to figure out how to limit a test to a given platform, and how to artificially make it run with a non-canonical cwd.

[FredKSchott]

I'm not sure why you say that this would affect a small number of users

It sounded like we wouldn't be able to test this in CI, did I misread that in your original post? That's where a lot of my caution here is coming from. Can you think of a way that we could write the test that would cover this behavior on either our Windows or Linux CI runner? It's fine if running the test in a case-insensitive env is essentially a no-op, as long as it's running at least once in the case-sensitive env.

[pkaminski]

All right, I wrangled together a test -- PTAL. Note that it's only effective in case-insensitive environments. Please let me know what else I can do to convince you that this is not some esoteric corner case. Thanks.

[FredKSchott]

LGTM with this comment addressed: https://github.com/pikapkg/snowpack/pull/999/files#r484578005

test looks great, thanks for adding. My fear here was entirely around not being able to test for this (that your PR worked, and that we didn't regress in the future), and not around it not being a valuable fix itself.

[pkaminski]

Cool, comment added. I didn't catch that your only concern was the test -- and it was a lot easier to put in than I feared. 😄 Thanks!