-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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(js): infer tslib as a dependency when using importHelpers #9350
feat(js): infer tslib as a dependency when using importHelpers #9350
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/nrwl/nx-dev/67zAs7EopJcbjXnbdWMcKYHPygaR [Deployment for f31486e canceled] |
2eaafcd
to
92585f6
Compare
@AgentEnder any reason CI is not running on this PR? |
First contribution right? For some reason, Circle seems to not be kicking off for first-time contributors. One of us will need to rebase your branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, going to get another opinion. Left a few small changes.
@@ -60,6 +61,8 @@ export async function* tscExecutor( | |||
options.tsConfig = tmpTsConfig; | |||
} | |||
|
|||
addTslibDependencyIfNeeded(options, context, dependencies); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AgentEnder does this seem like the right approach? Perhaps there's a way to make it so that tslib
will be part of the dependencies in the first place, by altering the graph somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine for the time being, eventually a lot of the js specific logic should be pulled out to use processProjectGraph
like a community plugin would, to truly separate the core and at that point then some of this may make more sense there. Since this is specific to the tsc
executor (and not wanted for the swc
executor), I think this makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I figured there's something missing to do what I had in mind. Thanks for the feedback 👍🏻 I'll push a fix momentarily
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉
When using "importHelpers" and building with "tsc", the built bundle expects to be supplied the "tslib" package. Currently, it's up to developers to add "tslib" as a dependency to those libraries' package.json files. This PR makes the "tsc" execute infer that "importHelpers" is used, and subsequently add "tslib" as a dependency to the generated package.json ISSUES CLOSED: nrwl#9343
dd9b35d
to
f31486e
Compare
This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request. |
Current Behavior
When using
"importHelpers": true
and building withtsc
, the built bundle expects to be supplied thetslib
package.Currently, it's up to developers to add
tslib
as a dependency to those libraries' package.json files.Expected Behavior
This PR makes the
tsc
executor infer thatimportHelpers
is used,and subsequently, add
tslib
as a dependency to the generatedpackage.json
.Related Issue(s)
Fixes #9343