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

feat: add exportConditions config to esinstall #1757

Merged
merged 2 commits into from
Dec 11, 2020

Conversation

ryansolid
Copy link
Contributor

@ryansolid ryansolid commented Dec 1, 2020

Changes

Rollup Plugin Node Resolve 11.0.0 adds support for custom export map resolution:
https://github.com/rollup/plugins/tree/master/packages/node-resolve#exportconditions

This is important for universal package aliasing for non-node environments. It's a blocker for using Snowpack as official dev tool in the SolidJS framework to enable SSR. I added this as a rollup config option the same as dedupe.

Discussion: #1758

Testing

This is a 2 liner change, mind you the update to rollup plugin node resolve 11.0.0 broke many tests before I even began adding the feature. It's possible that they've changed other resolution with the latest version.

Docs

I updated the Readme.md on esinstall. To my knowledge the specific options aren't currently documented anywhere.

@vercel
Copy link

vercel bot commented Dec 1, 2020

@ryansolid is attempting to deploy a commit to the Pika Team on Vercel.

A member of the Team first needs to authorize it.

@FredKSchott
Copy link
Owner

FredKSchott commented Dec 6, 2020

Hey! Thanks for the PR, I just read up on this new support in the Rollup plugin, looks good and +1 for updating to the v11 plugin to add this support by default.

Can you give more details about your use-case to customize this behavior in esinstall? Does the default not work for you for SSR specifically? Would love to understand your SolidJS use-case better!

(also rebasing on main should fix any breaking tests)

@ryansolid
Copy link
Contributor Author

ryansolid commented Dec 6, 2020

Solid supports multiple custom exports to support different types of bundles for different scenarios. It's a bit like Svelte in that there are custom compiled output for the components. But more so for seamless isomorphic experience I bundle different versions of the runtime. A single browser/node export is insufficient.

Solid supports full async SSR with isomorphic Suspense and automatic resource serialization/deserialization. Solid's server runtime differs whether it can be rendered synchronously (or async streaming) vs needing the full reactivity on the server to do async server rendering. While both versions support async SSR the non-reactive version of the runtime is extremely performant and beneficial to use for this case.

Rather than having to alias Solid's core libs and 3rd party libs each time if they wish to publish optimal versions, the 3rd party lib author could provide different outputs for each environment and the final application only needs to set the condition to pull in the right code. In our case server and server-async

Whether than can simply be from running the node process with the --conditions flag or bundling with it in rollup depends on the setup. To be fair the former with a custom dev server might make more sense reading the new docs. But at the time of making this PR I was thinking I'd need it in the bundled setup. It seems like a good option anyway to support any libs that might use custom exports like electron etc..

I can rebase when I get a chance.

Copy link
Owner

@FredKSchott FredKSchott left a comment

Choose a reason for hiding this comment

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

Got it, thanks for the extra details. So your main use-case is that you'd like to resolve different exports in your npm packages for the frontend build vs. the backend (SSR) build. Electron is a good call out as well, there are probably other scenarios where this applies.

+1 for getting this merged!

@@ -97,6 +97,7 @@ interface InstallOptions {
rollup: {
plugins?: RollupPlugin[];
dedupe?: string[];
exportConditions?: string[];
Copy link
Owner

Choose a reason for hiding this comment

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

can you move this up out of rollup and into the top-level InstallOptions?

We'll need to use this ourselves, since esinstall actually does some exports resolution logic as well. For example:

      const resolvedResult = resolveWebDependency(installSpecifier, {
        cwd,
        packageLookupFields,
      });

We actually already have support for this in packageLookupFields, but it's just not export map compliant. I actually wonder if it would be enough to use this same existing property (ex: ['svelte']) to inform both package.json top-level entrypoints AND valid export map entrypoints. I think we'd end up with something like this then:

- let foundEntrypoint: any = [...packageLookupFields, 'browser:module', 'module', 'main:esnext']
+ let packageEntrypointLookups: string[] = [...packageLookupFields, 'browser:module', 'module', 'main:esnext']
+ let packageExportLookups: string[] = [...packageLookupFields, 'browser', 'import', 'default']

what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm.. could do that although hard to say for sure. I guess people will use them the same way. You don't add electron or svelte unless you are looking to resolve those. Looking at https://nodejs.org/api/packages.html#packages_conditional_exports the stock ones are "import", "require", "node", "default".

Only thing is I think the plugin auto adds the defaults so you only need to add the extra's. I can use packageExportLookups fine enough. It works fine for my usage. Unclear if this affects anything else though. The tests still seem broken for me locally lets see how CI goes.

Copy link
Owner

Choose a reason for hiding this comment

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

yea that's fair, maybe these really should be two different installOptions: packageLookupFields and a new packageExportLookupFields. In your case you'll probably want to set both, but you may need to set both to different values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah to keep backward compatibility where export maps aren't supported, then the resolution could be different. Generally exports supersede the other entry form but we account for everyone's environment. I did a single option in the commit last night, but let's make it separate just in case. I can make that change tonight.

@vercel
Copy link

vercel bot commented Dec 7, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pikapkg/snowpack/f15c0tejk
✅ Preview: https://snowpack-git-export-conditions.pikapkg.vercel.app

Copy link
Owner

@FredKSchott FredKSchott left a comment

Choose a reason for hiding this comment

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

LGTM!

@FredKSchott
Copy link
Owner

Thanks again for this! Not sure why tests are failing, could just be an outdated snapshot, but I'll merge into main and then fix there for you if it still persists.

@FredKSchott FredKSchott merged commit 749a42d into FredKSchott:main Dec 11, 2020
@ryansolid
Copy link
Contributor Author

Awesome thank you!

@FredKSchott
Copy link
Owner

Unfortunately it looks like that new Rollup plugin version has issues.

Filed one here: rollup/plugins#727
But more exist as well: https://github.com/rollup/plugins/issues

I moved out the rollup package update, so this code is still merged in but effectively a no-op. We'll give that Rollup plugin some time to bake before updating our code to use it.

/cc @drwpow @matthewp who were also following these new Rollup plugins

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.

None yet

2 participants