-
Notifications
You must be signed in to change notification settings - Fork 508
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: don't replace lodash/fp imports with lodash-es/fp #884
Conversation
This comment has been minimized.
This comment has been minimized.
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.
Thanks for the bug report and fix @altrim ! I've proposed a cleaner fix below using RegEx.
While I've tested the RegEx I've provided against the source, it would also be good to add an automated test for this behavior to ensure it always works properly. A test could be added to test/e2e/tsdx-build-default.test.ts
with fixture in test/e2e/fixtures/build-default/src/index.ts
I'd also wonder if we should add a check for any other potential uses of lodash
packages, but afaict, lodash/fp
is the only other official one.
I've updated the PR to include the required changes. Anything else left or is this ok? @agilgur5 |
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.
Thanks for the iteration @altrim !
There's some small changes I'd like to see in the test, but would also be great if you could add tests for the original plain lodash
replacement to ensure that still works. Could write that as a separate PR though if you want since it's existing behavior.
Hey @agilgur5 anything else left here or is it ok to merge? |
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.
Great work on this @altrim ! Thanks for all the iterations
Hey @agilgur5 anything else left here or is it ok to merge?
If you're blocked by this, I'd recommend using patch-package
temporarily as until #254 is complete, I'm going to be batching releases so folks don't get spammed (even with #254 though, stable releases will still be batched, but canaries could go out faster).
474a675
to
c5e80ef
Compare
- previously, when importing modules from `lodash/fp`, the imports broke since `babel-plugin-transform-rename-import` renames all `lodash` imports to `lodash-es` for the ESM build - e.g `import mergeAll from 'lodash/fp/mergeAll';` ends up being imported as `import mergeAll from 'lodash-es/fp/mergeAll'` and we got an error since `lodash-es/fp` doesn't exist - with this fix we use a regex instead of bare string for replacement and add a negative lookahead to it so that it doesn't replace `lodash/fp` imports Co-authored-by: Anton Gilgur <[email protected]>
- lodash for CJS, lodash-es for ESM - previously, there were no tests for this behavior, so this adds them to ensure the replacement continues to work, even with the new fix with regex lookahead for `lodash/fp` Co-authored-by: Anton Gilgur <[email protected]>
c5e80ef
to
c9c6493
Compare
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 rebased to current master
and made some slight modifications to test layout and naming, but did not change any real content otherwise. Also squashed up some commits and added more details in the commit messages.
Apparently GitHub automatically marked my review as stale so re-adding it here. Will merge this as soon as CI tests pass!
@allcontributors please add @altrim for bug, code, test |
I've put up a pull request to add @altrim! 🎉 |
Ack, while writing a test for #896 I got this in the logs during a UMD build: No name was provided for external module 'lodash-es' in output.globals – guessing 'lodashEs'
No name was provided for external module 'lodash/fp' in output.globals – guessing 'fp'
No name was provided for external module 'lodash-es' in output.globals – guessing 'lodashEs'
|
When importing modules from
lodash/fp
we get breaking imports sincebabel-plugin-transform-rename-import
renames alllodash
imports tolodash-es
for the esm build.e.g
import mergeAll from 'lodash/fp/mergeAll';
ends up being imported likeimport mergeAll from 'lodash-es/fp/mergeAll'
and we get an error sincelodash-es/fp
doesn't exist.With this fix we rename all
lodash-es/fp
imports tolodash/fp
and we get the correct imports in the final esm build.