chore/issue 557 resolve node modules using node require resolve #674

thescientist13 · 2021-07-26T19:15:17Z

Related Issue

resolves #557

Summary of Changes

Updated plugin-node-modules to leverage require.resolve instead of assuming where node_modules is located on disk (in an effort to better handle npx commands showing error for not found node modules paths #505 / Bug/issue 505 improve default npx support #511 type situations)
Per the addition of testing in support of Shared Workspaces and Contexts (Theme Packs) #570, had to add some falback / exception handling in case require.resolve blows up like happened for our theme pack specs (should see if I can mock this better or make an issue / discussion)

Error: Cannot find module 'my-theme-pack'
Require stack:
- /Users/owenbuckley/Workspace/project-evergreen/repos/greenwood/packages/cli/src/plugins/resource/plugin-node-modules.js
- /Users/owenbuckley/Workspace/project-evergreen/repos/greenwood/packages/cli/src/lifecycles/config.js
- /Users/owenbuckley/Workspace/project-evergreen/repos/greenwood/packages/cli/src/lifecycles/compile.js
- /Users/owenbuckley/Workspace/project-evergreen/repos/greenwood/packages/cli/src/commands/build.js
- /Users/owenbuckley/Workspace/project-evergreen/repos/greenwood/packages/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 NodeModulesResource.resolve (/Users/owenbuckley/Workspace/project-evergreen/repos/greenwood/packages/cli/src/plugins/resource/plugin-node-modules.js:29:85)
    at /Users/owenbuckley/Workspace/project-evergreen/repos/greenwood/packages/cli/src/lifecycles/serve.js:4:421
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at async /Users/owenbuckley/Workspace/project-evergreen/repos/greenwood/packages/cli/src/lifecycles/serve.js:4:216
    at async /Users/owenbuckley/Workspace/project-evergreen/repos/greenwood/packages/cli/src/lifecycles/serve.js:4:216
    at async /Users/owenbuckley/Workspace/project-evergreen/repos/greenwood/packages/cli/src/lifecycles/serve.js:4:216
    at async /Users/owenbuckley/Workspace/project-evergreen/repos/greenwood/packages/cli/src/lifecycles/serve.js:4:216
    at async /Users/owenbuckley/Workspace/project-evergreen/repos/greenwood/packages/cli/src/lifecycles/serve.js:4:216 {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/Users/owenbuckley/Workspace/project-evergreen/repos/greenwood/packages/cli/src/plugins/resource/plugin-node-modules.js',
    '/Users/owenbuckley/Workspace/project-evergreen/repos/greenwood/packages/cli/src/lifecycles/config.js',
    '/Users/owenbuckley/Workspace/project-evergreen/repos/greenwood/packages/cli/src/lifecycles/compile.js',
    '/Users/owenbuckley/Workspace/project-evergreen/repos/greenwood/packages/cli/src/commands/build.js',
    '/Users/owenbuckley/Workspace/project-evergreen/repos/greenwood/packages/cli/src/index.js'
  ]
}

Initial and ongoing analysis and experiments being conducted in this repo - https://github.com/thescientist13/node-resolve-experiments

Was able to confirm things seem to be working as expected now that we are using require.resolve in the npx test case by changing the content to say <h2>hello require.resolve</h2>

% cd packages/cli/test/cases/build.default.quick-start-npx
owenbuckley@Owens-MBP-2 build.default.quick-start-npx % npx greenwood build
-------------------------------------------------------
Welcome to Greenwood (v0.14.1) ♻️
-------------------------------------------------------
Running Greenwood with the build command.
Initializing project config
Initializing project workspace contexts
Generating graph of workspace files...
Started local development server at localhost:1984
Prerendering pages at http://127.0.0.1:1984
pages to render
 src/pages/index.md
prerendering page... /
prerendering complete for page /.
done prerendering all pages
copying graph.json...
owenbuckley@Owens-MBP-2 build.default.quick-start-npx % cat public/index.html
<!DOCTYPE html><html lang="en" prefix="og:http://ogp.me/ns#">
                        <head>
                        <link rel="modulepreload" href="/header.4557af3c.js" as="script">
                      <!-- Shady DOM styles for app-header --><style>body {transition: opacity ease-in 0.2s; }
body[unresolved] {opacity: 0; display: block; overflow: hidden; position: relative; }
</style>





    <title>My App</title>
    <meta charset="utf-8">
    <meta name="viewport" content="width=device-width, initial-scale=1">
    <meta name="mobile-web-app-capable" content="yes">
    <meta name="apple-mobile-web-app-capable" content="yes">
    <meta name="apple-mobile-web-app-status-bar-style" content="black">


            <script type="module" src="/header.4557af3c.js"></script>

          </head>

  <body>


    <div>
      <app-header>
      <header class="style-scope app-header">This is the header component.</header>
    </app-header>
      <h2>hello require.resolve</h2>
    </div>



 </body></html>

TODO

Fix failing specs for Windows
Check for any other hardcoded lookup paths for node_modules when reading from the filesystem in Greenwood (I think ideally it should only need to happen in the plugin though)
- left some initial thoughts here - chore/issue 557 resolve node modules using node require resolve #674 (comment)
Update npx test case to not require unpkg fallback and / or figure out a way to test locally
Probably a little room left for some cleanup / refactoring
- not sure if we need to recursively support scoped packages? e.g. can they have names like @scoped-packages/something/else - reached out in NodeJS slack, will see, but I think it would definitely be an edge case
- wondering if we should change our logic here at all? Had some good lessons learned here that might be applicable in terms of prioritization and just make sure we have it right. 👌 - I think it is OK for now
- re-re-evaluate need for walkModule? - maybe for another PR / discussion

thescientist13

Might hold this back for the next release instead of going out with theme packs and 0.15.0 just to avoid too many big changes happening at once? Will see.

thescientist13 · 2021-08-04T18:28:13Z

Made another issue to evaluate pushing this idea forward more if it indeed does workout. 🙌
#684

thescientist13 · 2021-08-28T15:28:28Z

I wonder if this means we can remove all the setup work on our specs now that requires copy / pasting files out of root node_modules into the test cases directory? 🤔

edit: maybe for the files in node_modules, but I still think it's needed for at least package.json for Greenwood to build up the initial import map? I guess experimenting to test would be OK, but maybe that kind of refactoring can be saved for later? May be merge this and then alpha to validate?

thescientist13 added enhancement Improve something existing (e.g. no docs, new APIs, etc) CLI discussion tied to an ongoing discussion or meeting notes labels Jul 26, 2021

thescientist13 self-assigned this Jul 26, 2021

thescientist13 force-pushed the enhancement/issue-557-resolve-node-modules-using-node-require-resolve branch from 5c9d315 to 8b40245 Compare July 26, 2021 19:31

thescientist13 mentioned this pull request Jul 28, 2021

add import and directory support to export map subpath exports #672

Merged

thescientist13 marked this pull request as ready for review August 4, 2021 16:58

thescientist13 commented Aug 4, 2021

View reviewed changes

thescientist13 mentioned this pull request Aug 4, 2021

leverage import.meta.resolve to support import maps generation during development #684

Open

5 tasks

thescientist13 added chore unit testing, maintenance, etc todos and removed enhancement Improve something existing (e.g. no docs, new APIs, etc) labels Aug 4, 2021

thescientist13 force-pushed the enhancement/issue-557-resolve-node-modules-using-node-require-resolve branch from b168641 to 78ae84b Compare August 13, 2021 18:35

thescientist13 removed the todos label Aug 13, 2021

thescientist13 mentioned this pull request Aug 20, 2021

[WIP] example with Material Web Components ProjectEvergreen/greenwood-getting-started#42

Draft

thescientist13 added the question Further information is requested label Aug 28, 2021

thescientist13 added 6 commits September 3, 2021 12:48

initial POC

2849e81

all specs passing

f1c6ffb

update npx spec to remove fallback

78a780f

fix linting

9b2785c

add npx local testing docs to contributing guide

3b1a4db

add exception handling for nodejs based node modules lookup

30b5e19

thescientist13 force-pushed the enhancement/issue-557-resolve-node-modules-using-node-require-resolve branch from 78ae84b to 30b5e19 Compare September 3, 2021 17:06

thescientist13 merged commit 2f53deb into master Sep 3, 2021

thescientist13 deleted the enhancement/issue-557-resolve-node-modules-using-node-require-resolve branch September 3, 2021 17:23

thescientist13 changed the title ~~Enhancement/issue 557 resolve node modules using node require resolve~~ chore/issue 557 resolve node modules using node require resolve Sep 3, 2021

thescientist13 mentioned this pull request Sep 10, 2021

handle missed cases for files in exports maps and transient dependency imports #716

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore/issue 557 resolve node modules using node require resolve #674

chore/issue 557 resolve node modules using node require resolve #674

thescientist13 commented Jul 26, 2021 •

edited

Loading

thescientist13 left a comment

thescientist13 commented Aug 4, 2021

thescientist13 commented Aug 28, 2021 •

edited

Loading

chore/issue 557 resolve node modules using node require resolve #674

chore/issue 557 resolve node modules using node require resolve #674

Conversation

thescientist13 commented Jul 26, 2021 • edited Loading

Related Issue

Summary of Changes

TODO

thescientist13 left a comment

Choose a reason for hiding this comment

thescientist13 commented Aug 4, 2021

thescientist13 commented Aug 28, 2021 • edited Loading

thescientist13 commented Jul 26, 2021 •

edited

Loading

thescientist13 commented Aug 28, 2021 •

edited

Loading