Esinstall: Allow resolution of inner package files #1689

drwpow · 2020-11-24T00:16:37Z

Changes

Before

esinstall('react/LICENSE');

// [ERROR] Package " node_modules/react/LICENSE" not found. Have you installed it?

After

esinstall('react/LICENSE');

// ✅

Testing

Tested in local build setup. Tested against:

Docs

vercel · 2020-11-24T00:16:39Z

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/ogu613izn
✅ Preview: https://snowpack-git-drwpow-resolve-package-files.pikapkg.vercel.app

drwpow · 2020-11-24T01:01:25Z

esinstall/src/index.ts

@@ -302,7 +303,8 @@ export async function install(
        const aliasEntry = findMatchingAliasEntry(installAlias, specifier);
        return aliasEntry && aliasEntry.type === 'package' ? aliasEntry.to : specifier;
      })
-      .sort(),
+      .map((specifier) => specifier.replace(/(\/|\\)+$/, '')) // remove trailing slash from end of specifier (easier for Node to resolve)


This fixes a small but important problem: when using import maps, a . will resolve to preact/ when using path.join(). This makes esinstall generate preact/.js rather than preact.js.

Here, if a slash is the very last character(s), we simply remove it. Shouldn’t affect Node generation, and it seems to produce more consistently-named files on esinstall’s end

drwpow · 2020-11-24T01:27:28Z

esinstall/src/index.ts

+          const ext = path.basename(resolvedResult.loc).replace(/[^.]+/, '');
+          targetName += ext;
+          proxiedName += ext;
+        }


This fixes another bug where for non-JS assets, the extension was often stripped (e.g. when installing bulma, esinstall would create an extensionless web_modules/bulma file that was supposed to be web_modules/bulma.sass.

While this probably isn’t a perfect solution, I think it’s better, as it more closely matches the JS behavior. Also of note: we don’t add extensions if there aren’t any.

the thinking here makes sense, but resolvedResult.loc should already be a fully resolved file location. If we want to fix/handle this, the fix should live inside of resolveWebDependency()

So resolvedResult.loc is correct, and there‘s nothing to change there. That‘s why we correctly get the extension from there.

I was just making a change here where getWebDependencyName() and sanitizePackageName() were producing bad extensions for targetName and proxiedName for assets. Because we were adding asset handling here, I felt it safer to make the change here than in those 2 functions. But happy for you to make the call.

got it, makes sense!

FredKSchott

woo! 🎉 left a couple comments

FredKSchott · 2020-11-26T07:56:14Z

esinstall/src/index.ts

@@ -302,7 +303,8 @@ export async function install(
        const aliasEntry = findMatchingAliasEntry(installAlias, specifier);
        return aliasEntry && aliasEntry.type === 'package' ? aliasEntry.to : specifier;
      })
-      .sort(),
+      .map((specifier) => specifier.replace(/(\/|\\)+$/, '')) // remove trailing slash from end of specifier (easier for Node to resolve)
+      .sort((a, b) => a.localeCompare(b, undefined, {numeric: true})),


curious, why is this needed?

I think I default to localeCompare() more out of habit these days (it‘s recommended to do in browsers; probably doesn‘t make a difference in Node). But mainly I thought it‘d be a nice QoL improvement here to add {numeric: true}.

Whereas before it would sort file1.js, file10.js, file2.js, now it will sort file1.js, file2.js, file10.js.

FredKSchott · 2020-11-26T07:56:38Z

esinstall/src/index.ts

@@ -302,7 +303,8 @@ export async function install(
        const aliasEntry = findMatchingAliasEntry(installAlias, specifier);
        return aliasEntry && aliasEntry.type === 'package' ? aliasEntry.to : specifier;
      })
-      .sort(),
+      .map((specifier) => specifier.replace(/(\/|\\)+$/, '')) // remove trailing slash from end of specifier (easier for Node to resolve)


FredKSchott · 2020-11-26T07:58:29Z

esinstall/src/index.ts

+          const ext = path.basename(resolvedResult.loc).replace(/[^.]+/, '');
+          targetName += ext;
+          proxiedName += ext;
+        }


the thinking here makes sense, but resolvedResult.loc should already be a fully resolved file location. If we want to fix/handle this, the fix should live inside of resolveWebDependency()

drwpow · 2020-11-30T19:12:35Z

esinstall/src/rollup-plugins/rollup-plugin-node-process-polyfill.ts

@@ -6,6 +6,7 @@ const PROCESS_MODULE_NAME = 'process';
 export function rollupPluginNodeProcessPolyfill(env = {}): Plugin {
  const injectPlugin = inject({
    process: PROCESS_MODULE_NAME,
+    include: ['*.js', '*.mjs', '*.cjs'],


This is needed for Rollup to not blow up for non-JS assets.

drwpow · 2020-11-30T19:18:13Z

Snapshots seem to be failing due to minification differences; don‘t seem to be related to this PR at all.

FredKSchott

LGTM

drwpow force-pushed the drwpow/resolve-package-files branch from 388acff to 939a058 Compare November 24, 2020 00:59

vercel bot deployed to Preview November 24, 2020 00:59 View deployment

drwpow commented Nov 24, 2020

View reviewed changes

drwpow force-pushed the drwpow/resolve-package-files branch from 939a058 to ec351b4 Compare November 24, 2020 01:05

vercel bot deployed to Preview November 24, 2020 01:05 View deployment

drwpow force-pushed the drwpow/resolve-package-files branch from ec351b4 to e80871d Compare November 24, 2020 01:24

vercel bot deployed to Preview November 24, 2020 01:24 View deployment

drwpow commented Nov 24, 2020

View reviewed changes

vercel bot deployed to Preview November 24, 2020 21:25 View deployment

FredKSchott reviewed Nov 26, 2020

View reviewed changes

drwpow force-pushed the drwpow/resolve-package-files branch from 7563e2a to f44cb30 Compare November 28, 2020 01:56

vercel bot deployed to Preview November 28, 2020 01:56 View deployment

drwpow added 2 commits November 30, 2020 12:05

Allow resolution of inner package files

f028895

Prevent non-JS files from winding up in @rollup/plugin-inject

3c216cf

drwpow force-pushed the drwpow/resolve-package-files branch from f44cb30 to 3c216cf Compare November 30, 2020 19:05

vercel bot deployed to Preview November 30, 2020 19:05 View deployment

drwpow commented Nov 30, 2020

View reviewed changes

FredKSchott approved these changes Nov 30, 2020

View reviewed changes

drwpow merged commit c6abea7 into main Nov 30, 2020

drwpow deleted the drwpow/resolve-package-files branch November 30, 2020 21:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Esinstall: Allow resolution of inner package files #1689

Esinstall: Allow resolution of inner package files #1689

drwpow commented Nov 24, 2020

vercel bot commented Nov 24, 2020 •

edited

Loading

drwpow Nov 24, 2020

FredKSchott Nov 26, 2020

drwpow Nov 24, 2020

FredKSchott Nov 26, 2020

drwpow Nov 30, 2020 •

edited

Loading

FredKSchott Nov 30, 2020

FredKSchott left a comment •

edited

Loading

FredKSchott Nov 26, 2020

drwpow Nov 30, 2020 •

edited

Loading

FredKSchott Nov 26, 2020

FredKSchott Nov 26, 2020

drwpow Nov 30, 2020

drwpow commented Nov 30, 2020

FredKSchott left a comment

Esinstall: Allow resolution of inner package files #1689

Esinstall: Allow resolution of inner package files #1689

Conversation

drwpow commented Nov 24, 2020

Changes

Testing

Docs

vercel bot commented Nov 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drwpow Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FredKSchott left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drwpow Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drwpow commented Nov 30, 2020

FredKSchott left a comment

Choose a reason for hiding this comment

vercel bot commented Nov 24, 2020 •

edited

Loading

drwpow Nov 30, 2020 •

edited

Loading

FredKSchott left a comment •

edited

Loading

drwpow Nov 30, 2020 •

edited

Loading