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

Allow resolving subfolders with package.json files, and improve CJS interop #143

Merged
merged 3 commits into from
Jul 7, 2021

Conversation

calebeby
Copy link
Member

@calebeby calebeby commented Jul 7, 2021

There are two changes in here, but they overlapped in their code changes a little bit so it was easiest to open it as one PR instead of dealing with conflicts.

  1. Resolving folders with package.json: If I do import 'preact/hooks', that already works (before this PR), because Preact uses an exports map, which tells us how to resolve the ./hooks part. But many packages don't use exports maps yet, so they instead use the "old version" of this: in this case, they would create a hooks folder with a nearly-empty package.json in it, with a main or module field pointing to the file that they want to import. Before this PR, pleasantest didn't support this (it only traversed through the main package.json, e.g. node_modules/preact/package.json). Now, if an import resolves to a folder, recursion happens to resolve through a package.json in that folder (if it exists).
  2. Improved CJS interop: There are packages on npm that are CJS-only, and they make it really really hard to statically determine their exports. Because of our bundling step, we need to be able to statically determine named exports of packages at compile-time. For some packages (react, react-dom, prop-types), they are really difficult to determine. Here's what the entry points for react and react-dom look like:
    'use strict';
    if (process.env.NODE_ENV === 'production') {
      module.exports = require('./cjs/react.production.min.js');
    } else {
      module.exports = require('./cjs/react.development.js');
    }
    Obviously, by just looking at that file, you can't figure out which names are exported. Replace vite module server with wmr/custom module server #115 implemented a rollup transform that would detect the module.exports = require(... lines, and would then read the require'd files and statically determine the exports from those. But for prop-types, it doesn't work because there is a function call which makes the static analysis of the require'd file break:
    if (process.env.NODE_ENV !== 'production') {
      var throwOnDirectAccess = true;
      module.exports = require('./factoryWithTypeCheckers')(ReactIs.isElement, throwOnDirectAccess);
    } else {
      module.exports = require('./factoryWithThrowingShims')();
    }
    In this PR, I deleted the static analysis for module.exports = require(..., and replaced it with an include-list of modules that should be require'd (at compile time) to determine their exports. This is used for prop-types, react, and react-dom. We can easily add more to that list down the road. If facebook eventually decides to add ESM to their packages, this should continue working without the compile-time require because it checks if the package is CJS before requiring it.

package.json Show resolved Hide resolved
@calebeby
Copy link
Member Author

calebeby commented Jul 7, 2021

BTW: The CJS interop stuff has tests for it, but the resolution stuff does not.

In a later PR, I'm planning to separate two files out of the npm plugin: 1 for the bundler thing that calls rollup, and one for the nodeResolve function. Then I'll write isolated tests for the nodeResolve function (and probably fix more edge cases)

Copy link
Member

@Paul-Hebert Paul-Hebert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code all looks good to me, but I feel like I'm lacking some context into how this interacts with the rest of Pleasantest. Depending on how thorough of a review you need you may want to tag in another dev for a second pair of eyes.

src/module-server/plugins/npm-plugin.ts Show resolved Hide resolved
src/module-server/plugins/npm-plugin.ts Outdated Show resolved Hide resolved
Copy link
Member

@gerardo-rodriguez gerardo-rodriguez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't have a full grasp of everything but code LGTM! 👍

@calebeby calebeby merged commit 0ba7584 into main Jul 7, 2021
@calebeby calebeby deleted the fix-resolution-and-improve-cjs-interop branch July 7, 2021 22:48
@github-actions github-actions bot mentioned this pull request Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants