Fix import sources needing trailing slash for file aliases

[novaluke]

Changes

Previously it wasn't possible to have an import alias that pointed to a specific file, instead of a directory. The only way to make it work was to include a trailing slash in the import specifier, which is undesirable.

// snowpack.config.js
module.exports = {
  alias: {
    "@foo": "./src/foo.js",
  },
};

// src/bar.js
import foo from "@foo"; // doesn't work, but should
import foo from "@foo/"; // does work, but import shouldn't have a trailing slash

This change makes it so that aliases to specific files don't require a trailing slash (ie. import "@foo" now works).

Testing

I have added a test case to the resolve-imports test file and updated the snapshot to match the new behavior.

Note: jest -u wanted to remove a bunch of obsolete snapshots, but I chose not to commit those deletions since that's beyond the scope of this PR. Also I only tested the resolve-imports tests, since the rest of the tests failed on a fresh install on my machine and I couldn't figure out why.

Docs

Public documentation was not updated, as this bug fix restores the behavior to what a user would presumably expect based on the existing docs.

[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/72qz0q6ma
✅ Preview: https://snowpack-git-fix-import-specifiers-for-file-aliases.pikapkg.vercel.app

[novaluke]

Looks like the Windows build failed, but as far as I can tell it's not related to the changes in this PR. Let me know if I need to do anything to fix it though!

[drwpow]

Note: jest -u wanted to remove a bunch of obsolete snapshots, but I chose not to commit those deletions since that's beyond the scope of this PR. Also I only tested the resolve-imports tests, since the rest of the tests failed on a fresh install on my machine and I couldn't figure out why.

Yeah I apologize about that. We have some snapshot tests which are overdue for a cleanup, so you are free to ignore those. Thanks for calling attention to it!

[drwpow]

This change looks fine to me! I agree that aliasing should work for files as well as folders. If I had to guess as to why this behavior was here in the first place, I’d guess it was carried over from our mount configuration, which by its nature must enforce that only directories be mounted. Which doesn’t make sense for alias.

Overall I’m +1 in favor of this change, but as this would change behavior for some existing setups, I’d also like to suggest on util.ts#L295: removing the removeTrailingSlash(from) check, which I assume is undoing the operation here in config.ts.

Additionally, while I’d like to add some new error message like (Could not find [myalias]. Did you mean [myalias]/?), this logic takes place in a greater context of us trying several things to resolve an import and failing silently. So I can’t really think of a place where surfacing an error message would make any sense. So in the case of someone relying on this behavior, I want to say that the error message itself should be clear enough on its own.

But backing up to the original intent of this PR, I don’t even think we have to worry about that because rarely will someone try and import a directory anyway.

[drwpow]

Requested a change to util.ts as well (see comment), unless that causes other unexpected behavior / throws tests.

[novaluke]

[remove] the removeTrailingSlash(from) check, which I assume is undoing the operation here

I had wondered about that as well, but wasn't sure what it might break due to my lack of understanding of the code as a whole. The tests still pass after removing it, so I've pushed that change as well now.

[drwpow]

Looks great! Thank you!

[novaluke]

Thanks @drwpow! 🎉

[FredKSchott]

Note that we should consider this a breaking change for existing users. We'll need to do some more cleanup here before releasing a new version