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

leverage NodeJS resolve logic for looking up location of paths for node modules #557

Closed
1 of 5 tasks
thescientist13 opened this issue Apr 16, 2021 · 6 comments · Fixed by #674, #722, #725 or #733
Closed
1 of 5 tasks
Assignees
Labels
Milestone

Comments

@thescientist13
Copy link
Member

thescientist13 commented Apr 16, 2021

Type of Change

  • New Feature Request
  • Documentation / Website
  • Improvement / Suggestion
  • Bug
  • Other (please clarify below)

Summary

As part of looking into #505 as part of submitting #511 and so for then, was just getting something done that was quick and dirty, using unpkg.com as a fallback. I'm not sure if this will support everything like things added to importMap... but I did reach out in NodeJS slack channel and got this tip on using NodeJS require.resolve and so that would certainly be interesting because if we could piggy back somehow of off Node's default resolve behavior, that would allow us to maintain all that on our own.

This would also ensure that when Greenwood is run from the command line using npx, node_modules would always resolve correctly.

Details

So essentially, give it a package name and it will look up the path for you.

// /Users/owenbuckley/Workspace/project-evergreen/repos/greenwood/packages/cli/node_modules/es-module-shims/dist/es-module-shims.js
console.debug( require.resolve('es-module-shims') )

With a better local testing environment (using npm|yarn link), we could set it up so that it in plugins-node-modules resolve just delegates all this to require.resolve instead:

// example url => /node_modules/es-module-shims/dist/es-module-shims.js
async resolve(url) {
  return new Promise((resolve, reject) => {
    try {
      const packageName = url.split('/')[1]; // ex. es-module-shims
      const packageFullPath = require.resolve(packageName); // ex. /Users/.../node_modules/es-module-shims/dist/es-module-shims.js
      const currentDirectory = packageFullPath.split('/node_modules/')[0]; //ex. /Users/.../
      const nodeModulesUrl = path.join(currentDirectory, url.replace.replace('/node_modules/'); //ex. /Users/.../es-module-shims/dist/es-module-shims.js

      resolve(nodeModulesUrl);
    } catch (e) {
      console.error(e);
      reject(e);
    }
  });
}

We might want to generate our own full path since the url is what we want based on walking package.json (in theory) as opposed to what NodeJS thinks we want.


Given that, perhaps there is a case for us to entirely refactor getPackageEntryPath to be based off require.resolve logic instead? 🤔

We intentionally always want to favor ESM if we can, so I guess it depends on if anything would break but we certainly have enough tests for that. 😃


Another consideration might be around when to specify extensions being added to files, like the browser does, as currently right now for anything from node_modules it is not required, but any components or JS in your project do. Maybe a case for a plugin that will handle resolving extensions a la webpack?

@thescientist13 thescientist13 added the chore unit testing, maintenance, etc label Apr 16, 2021
@thescientist13 thescientist13 added this to the 1.0 milestone Apr 16, 2021
@thescientist13 thescientist13 self-assigned this Apr 16, 2021
@thescientist13 thescientist13 added the question Further information is requested label Apr 16, 2021
@thescientist13 thescientist13 added the help wanted Extra attention is needed label Jun 30, 2021
@thescientist13
Copy link
Member Author

thescientist13 commented Jul 11, 2021

As part of the WCCG, starting a repo for experimenting with this.
https://github.com/thescientist13/node-resolve-expirements

@thescientist13 thescientist13 added CLI enhancement Improve something existing (e.g. no docs, new APIs, etc) and removed chore unit testing, maintenance, etc labels Jul 26, 2021
@thescientist13 thescientist13 changed the title leverage NodeJS resolve logic for looking up location of files in node modules leverage NodeJS resolve logic for looking up location of paths for node modules Jul 26, 2021
@thescientist13 thescientist13 added chore unit testing, maintenance, etc and removed enhancement Improve something existing (e.g. no docs, new APIs, etc) labels Aug 4, 2021
@thescientist13
Copy link
Member Author

Will likely want to do an alpha release for this change and make sure to do tests for

  • npx
  • Stackblitz
  • downstream projects

@thescientist13
Copy link
Member Author

Cool, the npx steps from the home page work now without any of those unpkg work arounds now!
Screen Shot 2021-09-10 at 5 41 41 PM

@thescientist13
Copy link
Member Author

Observed as part of #716 that were still some critical hard coded paths assuming root level location of _node_modules, so will be re-opening this to try and clean those instances up.

@thescientist13 thescientist13 removed help wanted Extra attention is needed question Further information is requested labels Sep 13, 2021
@thescientist13
Copy link
Member Author

thescientist13 commented Sep 20, 2021

Hmm, found a regression in a Lit@2 repo where require.resolve was not finding @types/trusted-types, but we had no proper fallback logic for that case

Unable to look up package using NodeJS require.resolve for => @types/trusted-types Error: Cannot find module '@types/trusted-types'
Require stack:
- /Users/owenbuckley/Workspace/contributary/repos/www.contributary.community/node_modules/@greenwood/cli/src/lib/node-modules-utils.js
- /Users/owenbuckley/Workspace/contributary/repos/www.contributary.community/node_modules/@greenwood/cli/src/plugins/resource/plugin-node-modules.js
- /Users/owenbuckley/Workspace/contributary/repos/www.contributary.community/node_modules/@greenwood/cli/src/lifecycles/config.js
- /Users/owenbuckley/Workspace/contributary/repos/www.contributary.community/node_modules/@greenwood/cli/src/lifecycles/compile.js
- /Users/owenbuckley/Workspace/contributary/repos/www.contributary.community/node_modules/@greenwood/cli/src/commands/build.js
- /Users/owenbuckley/Workspace/contributary/repos/www.contributary.community/node_modules/@greenwood/cli/src/index.js
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:880:15)
    at Function.resolve (internal/modules/cjs/helpers.js:94:19)
    at getNodeModulesResolveLocationForPackageName (/Users/owenbuckley/Workspace/contributary/repos/www.contributary.community/node_modules/@greenwood/cli/src/lib/node-modules-utils.js:7:42)
    at getPackageEntryPath (/Users/owenbuckley/Workspace/contributary/repos/www.contributary.community/node_modules/@greenwood/cli/src/plugins/resource/plugin-node-modules.js:36:49)
    at /Users/owenbuckley/Workspace/contributary/repos/www.contributary.community/node_modules/@greenwood/cli/src/plugins/resource/plugin-node-modules.js:102:19
    at Array.forEach (<anonymous>)
    at walkPackageJson (/Users/owenbuckley/Workspace/contributary/repos/www.contributary.community/node_modules/@greenwood/cli/src/plugins/resource/plugin-node-modules.js:98:47)
    at /Users/owenbuckley/Workspace/contributary/repos/www.contributary.community/node_modules/@greenwood/cli/src/plugins/resource/plugin-node-modules.js:186:9
    at Array.forEach (<anonymous>)
    at walkPackageJson (/Users/owenbuckley/Workspace/contributary/repos/www.contributary.community/node_modules/@greenwood/cli/src/plugins/resource/plugin-node-modules.js:98:47) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/Users/owenbuckley/Workspace/contributary/repos/www.contributary.community/node_modules/@greenwood/cli/src/lib/node-modules-utils.js',
    '/Users/owenbuckley/Workspace/contributary/repos/www.contributary.community/node_modules/@greenwood/cli/src/plugins/resource/plugin-node-modules.js',
    '/Users/owenbuckley/Workspace/contributary/repos/www.contributary.community/node_modules/@greenwood/cli/src/lifecycles/config.js',
    '/Users/owenbuckley/Workspace/contributary/repos/www.contributary.community/node_modules/@greenwood/cli/src/lifecycles/compile.js',
    '/Users/owenbuckley/Workspace/contributary/repos/www.contributary.community/node_modules/@greenwood/cli/src/commands/build.js',
    '/Users/owenbuckley/Workspace/contributary/repos/www.contributary.community/node_modules/@greenwood/cli/src/index.js'
  ]
}

Screen Shot 2021-09-20 at 5 15 52 PM

it is a dependency of lit-html

% yarn why @types/trusted-types
yarn why v1.22.5
[1/4] 🤔  Why do we have the module "@types/trusted-types"...?
[2/4] 🚚  Initialising dependency graph...
[3/4] 🔍  Finding dependency...
[4/4] 🚡  Calculating file sizes...
=> Found "@types/[email protected]"
info Reasons this module exists
   - "lit#lit-html" depends on it
   - Hoisted from "lit#lit-html#@types#trusted-types"
info Disk size without dependencies: "16KB"
info Disk size with unique dependencies: "16KB"
info Disk size with transitive dependencies: "16KB"
info Number of shared dependencies: 0
✨  Done in 0.54s.

Will try and narrow down why this is an issue now, but seems related to #725

@thescientist13
Copy link
Member Author

Found found the root cause? It seems that packages with no main field or an empty main field bomb out on usage of require.resolve.

// index,js
require.resolve('@babel/runtime')
node index.js
LIT 111 /Users/owenbuckley/Workspace/contributary/repos/www.contributary.community/node_modules/lit-html/lit-html.js
Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: No "exports" main defined in /Users/owenbuckley/Workspace/contributary/repos/www.contributary.community/node_modules/@babel/runtime/package.json
    at throwExportsNotFound (internal/modules/esm/resolve.js:290:9)
    at packageExportsResolve (internal/modules/esm/resolve.js:513:3)
    at resolveExports (internal/modules/cjs/loader.js:432:36)
    at Function.Module._findPath (internal/modules/cjs/loader.js:472:31)
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:867:27)
    at Function.resolve (internal/modules/cjs/helpers.js:94:19)
    at Object.<anonymous> (/Users/owenbuckley/Workspace/contributary/repos/www.contributary.community/index.js:3:39)
    at Module._compile (internal/modules/cjs/loader.js:1063:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1092:10)
    at Module.load (internal/modules/cjs/loader.js:928:32) {
  code: 'ERR_PACKAGE_PATH_NOT_EXPORTED'
}
// index,js
require.resolve('@types/trusted-types');
% node index.js
Error: Cannot find module '@types/trusted-types'
Require stack:
- /Users/owenbuckley/Workspace/contributary/repos/www.contributary.community/index.js
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:880:15)
    at Function.resolve (internal/modules/cjs/helpers.js:94:19)
    at Object.<anonymous> (/Users/owenbuckley/Workspace/contributary/repos/www.contributary.community/index.js:4:39)
    at Module._compile (internal/modules/cjs/loader.js:1063:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1092:10)
    at Module.load (internal/modules/cjs/loader.js:928:32)
    at Function.Module._load (internal/modules/cjs/loader.js:769:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:72:12)
    at internal/main/run_main_module.js:17:47 {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/Users/owenbuckley/Workspace/contributary/repos/www.contributary.community/index.js'
  ]
}

Maybe this is something that will be accounted for as part of ESM migration, by honoring export maps at least - #707

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment