-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
fix(node): webpack/terser stripping class names breaking reflection #8784
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/nrwl/nx-dev/J2u4N7w1s1ReHFor8DfB3vGUA7TD [Deployment for 6278462 canceled] |
LGTM 🎉 🚀 . Minification / obfuscation makes less sense on the server anyways, but it certainly shouldn't be there if its breaking things. |
@IamBlueSlime do you care to try rebasing the PR and pushing it back up? For some reason Circle CI doesn't always kick off that build job. |
Head branch was pushed to by a user without write access
☁️ Nx Cloud ReportCI ran the following commands for commit 6278462. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch ✅ Successfully ran 7 targets
Sent with 💌 from NxCloud. |
Done 👌 But no trace of CircleCI :/ |
Class names are necessary for frameworks using reflection internally. Like Nest.js which uses reflection when injecting dependency.
@IamBlueSlime I rebased and force-pushed your branch. CI should kick in now. |
Hi! Should we also give the same treatment to |
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 building an app with
optimization: true
, Terser will be instanciated in Webpack's configuration which will apply some optimization mechanisms like minifying the code, etc...But it will also strip most class names if they are not referenced elsewhere. Thus breaking any reflection mechanism.
Example 1:
In the context of a Nest.js migration using TypeORM, if your migration is as simple as it seems, it will be only referenced when instantiating the Nest's
TypeORMModule
. As the class is only referenced in themigrations
array, Terser will remove the class's name as it seems unneeded. Unfortunately, the class name is used by TypeORM to recognize the migration name and timestamp. TypeORM is blocked.Example 2:
Still in a Nest.js context, when using a class as a provider, Nest will use the class name as the provider symbol for further injection. Stripping the class name can either (1) prevent Nest from injecting the provider correctly or, worse (2), inject another class which differs from the one intended.
In my workspace, I've disabled the optimizations by setting
optimization: false
but Webpack will then produce a development bundle, still interpolating some values likeNODE_ENV
by hardcoding it todevelopment
. This same bundle cannot be used in production because it is well known that some dependencies use conditionals likeif (NODE_ENV == 'production')
. So my company is blocked: either our apps won't boot or they cannot be deployed to production because their behaviors could be undefined.IMO, I consider this a critical issue.
Expected Behavior
In the best world, Terser should be smart enough, but this is highly runtime code which is really hard to find statically. Here I'm telling Terser to keep all the class names which will have an impact on the bundle size, but as the current version of Nx is breaking most (to not say all) Nest apps, I consider this a non-issue.
Do not hesitate to challenge my fix proposal, but keep in mind that what can be done on a second time should be done in another PR, to ship this fix ASAP.
To avoid any issue in the future, it could be relevant to add a dummy Nest app as a e2e test to ensure reflection still works.
Related Issue(s)
I guess it fixes #8441 and fixes #8446, but it also shows that #8537 was not fixed properly, I guess this PR fixes it too.