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

Fixed PnP compatibility for bundled components package #18015

Merged
merged 1 commit into from May 11, 2022
Merged

Conversation

Andarist
Copy link
Contributor

hopefully, fixes #17978 caused by regression from #17947

  1. util-deprecate has package.json#browser
  2. qs depends on object-inspect and that package has package.json#browser

We can't bundle anything that has an alias field like that, such packages should always be external to leave the aliased stuff untouched for the consumers so they can resolve this on their own, based on the consuming environment.

@nx-cloud
Copy link

nx-cloud bot commented Apr 20, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 9b0a59c. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@@ -21,11 +21,19 @@ interface Options {
watch?: boolean;
}

const makeExternalPredicate = (externals: string[]) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might matter for some packages so you don't end up bundling stuff like lodash/pickBy, because without a solution like this 'lodash' is treated as an external but all internal submodules are just bundled

Comment on lines +24 to +30
const makeExternalPredicate = (externals: string[]) => {
if (externals.length === 0) {
return () => false;
}
const pattern = new RegExp(`^(${externals.join('|')})($|/)`);
return (id: string) => pattern.test(id);
};
Copy link
Member

Choose a reason for hiding this comment

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

this returns a fn?

I guess rollup supports this?
Why is this better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this returns a fn?

yes, this returns a predicate function testing if a particular import source should be treated as external or not

I guess rollup supports this?

yes

Why is this better?

#18015 (comment) , basically without this you have such a situation:

import _ from 'lodash' // treated as external
import pickBy from 'lodash/pickBy' // bundled in your script

The external match is exact so anything that you import using a "deep import" will be bundled (by default). This kind of predicate fixes the problem.

@Andarist
Copy link
Contributor Author

Andarist commented May 9, 2022

@ndelangen conflicts resolved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Yarn pnp incompatibility
2 participants