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

Bug/issue 505 improve default npx support #511

Merged
merged 6 commits into from
Apr 17, 2021

Conversation

thescientist13
Copy link
Member

@thescientist13 thescientist13 commented Apr 8, 2021

Related Issue

resolves #505

Summary of Changes

  1. Add fallback detection to unpkg.com if internal client side dependencies for es-module-shims and @webcomponents/webcomponentjs aren't found in process.cwd()
  2. Add npx docs back to the home page and quick start
  3. Add a test specifically to omit those dependencies from scaffolding (is just a copy / paste of default case)
  4. Whitelist unpkg.com with puppeteer

TODO

  1. noticed a bug when trying to merge user page templates while still using default app template, so will want to make a bug and fix that first, then can enable the xdescribe spec here and then we can merge.
  2. rebase after Bug/issue 512 default app template merge fixes #513 is merged

Notes

For now, just getting this quick and dirty happy path working, using unpkg.com as a fallback. But I did reach out in NodeJS slack channel and got this tip on using NodeJS require.resolve.

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 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.


That said, 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. 😃

I'll definitely be making a follow up issue for us to explore this more so we can try and provide the best npx experience possible as part of hitting 1.0, and not have to worry ourselves about all this node_modules look up stuff if we can help it.

@thescientist13 thescientist13 added bug Something isn't working question Further information is requested website Tasks related to the projects website / documentation P0 Critical issue that should get addressed ASAP labels Apr 8, 2021
@thescientist13 thescientist13 self-assigned this Apr 8, 2021
@thescientist13
Copy link
Member Author

Open #557 to keep the conversation going forward and making sure we can ensure the best (npx) experience possible when integrating with npm packages.

@thescientist13 thescientist13 added documentation Greenwood specific docs and removed question Further information is requested website Tasks related to the projects website / documentation labels Apr 16, 2021
@thescientist13 thescientist13 force-pushed the bug/issue-505-improve-default-npx-support branch from 211e7ea to c2b1ab2 Compare April 16, 2021 01:21
@thescientist13 thescientist13 merged commit 8cc9119 into master Apr 17, 2021
@thescientist13 thescientist13 deleted the bug/issue-505-improve-default-npx-support branch April 17, 2021 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CLI documentation Greenwood specific docs P0 Critical issue that should get addressed ASAP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

npx commands showing error for not found node modules paths
1 participant