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

[Feature] Support custom export resolution strategy #2815

Closed
2 tasks done
SamChou19815 opened this issue Apr 27, 2021 · 6 comments
Closed
2 tasks done

[Feature] Support custom export resolution strategy #2815

SamChou19815 opened this issue Apr 27, 2021 · 6 comments
Labels
enhancement New feature or request

Comments

@SamChou19815
Copy link
Contributor

SamChou19815 commented Apr 27, 2021

  • I'd be willing to implement this feature (contributing guide)
  • This feature is important to have in this repository; a contrib plugin wouldn't do

Describe the user story

Recently yarn landed the support for exports in package.json. Currently it hardcodes the resolution to support cjs only:

const resolvedExport = resolveExport(pkgJson, ppath.normalize(subpath), {
browser: false,
require: true,
// TODO: implement support for the --conditions flag
// Waiting on https://github.com/nodejs/node/issues/36935
conditions: [],
});

I understand that ESM support is not ready yet (#638). However, the ability to resolve to esm module is still important, since Yarn pnp will also be used to resolve files, and the execution might be handled by something else (e.g. browser). For example, esbuild has a platform flag that can be set to browser, and it is currently not respected by Yarn esbuild pnp plugin. Yarn will always use the main field, and in the presence of exports, use the require one, so I have to fork it and do extra work to make it support browser field:

https://github.com/SamChou19815/website/blob/ca0f70ff61778107a6cdc456d65b3ba712bdb57b/packages/esbuild-scripts/esbuild/esbuild-pnp-plugin.ts#L41-L53

It would be nice if pnpapi could expose an additional flag to customize the behavior.

Describe the solution you'd like

In the opts argument of resolveRequest and , we can add another optional flag called platform that defaults to node.

If the field platform is node, then we keep the old behavior. If the field platform is browser, then we try to look into main fields in this order: ['browser', 'module', 'main'] without exports and make the argument passed to resolve.exports to be { browser: true, require: false }.

Describe the drawbacks of your solution

It will increase the complexity of the pnpapi, and the browser field, although recognized by resolve.exports, is not part of the node standard.

Describe alternatives you've considered

Add extra postprocessing step after pnpapi resolution, like the hacky one I used in https://github.com/SamChou19815/website/blob/ca0f70ff61778107a6cdc456d65b3ba712bdb57b/packages/esbuild-scripts/esbuild/esbuild-pnp-plugin.ts#L41-L53.

@SamChou19815 SamChou19815 added the enhancement New feature or request label Apr 27, 2021
@arcanis
Copy link
Member

arcanis commented Apr 27, 2021

I'll look in more details later, but note that resolveRequest is an "helper" function that in theory isn't meant to be the primary entry point of the PnP API. Ideally, you'd call resolveToUnqualified in order to turn a bare identifier (lodash/something) into a file directory (/path/to/lodash/something), and let whatever is the high-level resolver turn that into a proper fully-qualified path (/path/to/lodash/something/index.js). In this scenario, it would be the high-level resolver that would be tasked from resolving the exports field, not the PnP API.

Now to answer the next two questions that you might ask:

  • Then why do we support the exports field in the PnP API at all? Because Node has a few flaws in its resolution pipeline. In particular, it uses its own internal fs functions that we cannot extend with the zip layer. As a result, we have to reimplement the whole resolution 🙁

  • Then why do we use resolveRequest in the esbuild plugin, rather than resolveToUnqualified? Because ESBuild doesn't yet expose the "default resolver" to which we can forward the partial resolution once we have it (contrary to enhanced-resolve, for example, which adds PnP at a specific point in the resolution pipeline). As a result, we also have to reimplement the whole resolution.

@SamChou19815
Copy link
Contributor Author

@arcanis Following your advice, I reimplemented the pnp resolution plugin for esbuild using resolveToUnqualified here: SamChou19815/website#1189, and it seems to work for my limited set of websites.

The main change is to use resolve.exports's resolve, legacy to do resolution on packages. It will use different options depending on the browser flag.

https://github.com/SamChou19815/website/blob/2379267e7b9f0bdfb710faf39ee79bc371a421df/packages/esbuild-scripts/esbuild/pnp-resolution.ts#L16-L29

Then I forked the applyNodeExportsResolution function to use the customized exports resolver defined above:

https://github.com/SamChou19815/website/blob/2379267e7b9f0bdfb710faf39ee79bc371a421df/packages/esbuild-scripts/esbuild/pnp-resolution.ts#L56

Therefore, if we want to build the support for browser-like resolution in pnpapi core, I think we should add an optional field like platform in resolveRequest's option (also updated the proposal above). Even if we don't plan to add the support in pnpapi, I think it's still helpful to do similar things in the official @yarnpkg/esbuild-plugin so that it can be used for web bundling without modification.

@SteffeyDev
Copy link

SteffeyDev commented Jun 7, 2021

Thanks for your work @SamChou19815! What do you think would it take to get your changes merged into @yarnpkg/esbuild-plugin-pnp?

@arcanis
Copy link
Member

arcanis commented Jun 7, 2021

Thanks for reminding me! Yes, it definitely makes sense to fix that in the plugin, at least until ESBuild provides us way to call into its default resolver to leverage its logic (which is for example how it works in Webpack - the PnP code just handles the bare import resolution, and the native Webpack resolver then continue on its own).

We wouldn't duplicate the code though, so I think the ideal way would be to add a condition option to resolveRequest and resolveUnqualified for this purpose, as you suggested (I guess we don't need browser if we have conditions, correct?).

@SteffeyDev
Copy link

SteffeyDev commented Jun 7, 2021

Conditions would be a great start, but wouldn't account for edge-cases where older or not-yet-updated versions of packages have a "browser" field but no "exports" field. (e.g. uuid@3.4.0, which is still depended on by certain packages)

@merceyz
Copy link
Member

merceyz commented Jan 22, 2022

In #2161 I added support for passing conditions to resolveRequest and resolveUnqualified.

module and browser support in the ESBuild plugin can be tracked in #3185 and #2987

@merceyz merceyz closed this as completed Jan 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants