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

fix: don't replace lodash/fp imports with lodash-es/fp #884

Merged
merged 2 commits into from
Oct 5, 2020

Conversation

altrim
Copy link
Contributor

@altrim altrim commented Sep 21, 2020

When importing modules from lodash/fp we get breaking imports 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 like import mergeAll from 'lodash-es/fp/mergeAll' and we get an error since lodash-es/fp doesn't exist.

With this fix we rename all lodash-es/fp imports to lodash/fp and we get the correct imports in the final esm build.

@vercel

This comment has been minimized.

Copy link
Collaborator

@agilgur5 agilgur5 left a 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.

src/babelPluginTsdx.ts Outdated Show resolved Hide resolved
src/babelPluginTsdx.ts Outdated Show resolved Hide resolved
@altrim
Copy link
Contributor Author

altrim commented Sep 24, 2020

I've updated the PR to include the required changes. Anything else left or is this ok? @agilgur5

Copy link
Collaborator

@agilgur5 agilgur5 left a 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.

package.json Outdated Show resolved Hide resolved
test/e2e/fixtures/build-default/src/index.ts Outdated Show resolved Hide resolved
test/e2e/tsdx-build-default.test.ts Outdated Show resolved Hide resolved
test/e2e/tsdx-build-default.test.ts Outdated Show resolved Hide resolved
test/e2e/tsdx-build-default.test.ts Outdated Show resolved Hide resolved
test/e2e/tsdx-build-default.test.ts Outdated Show resolved Hide resolved
@altrim
Copy link
Contributor Author

altrim commented Sep 28, 2020

Hey @agilgur5 anything else left here or is it ok to merge?

agilgur5
agilgur5 previously approved these changes Oct 1, 2020
Copy link
Collaborator

@agilgur5 agilgur5 left a 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).

@agilgur5 agilgur5 changed the title Fix lodash/fp imports. fix: don't replace lodash/fp imports with lodash-es/fp Oct 1, 2020
@agilgur5 agilgur5 added the kind: bug Something isn't working label Oct 1, 2020
Altrim Beqiri and others added 2 commits October 5, 2020 17:00
- 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 <agilgur5@gmail.com>
- 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 <agilgur5@gmail.com>
Copy link
Collaborator

@agilgur5 agilgur5 left a 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!

@agilgur5 agilgur5 merged commit 9c4ce68 into jaredpalmer:master Oct 5, 2020
@agilgur5
Copy link
Collaborator

agilgur5 commented Oct 5, 2020

@allcontributors please add @altrim for bug, code, test

@allcontributors
Copy link
Contributor

@agilgur5

I've put up a pull request to add @altrim! 🎉

@agilgur5 agilgur5 added this to the v0.14.x milestone Oct 5, 2020
@agilgur5
Copy link
Collaborator

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'

lodash-es I assume would use the same global lodash (or maybe UMD shouldn't use the -es variant #759 ), not sure what lodash/fp should use... jotting this down, haven't quite looked into it yet.
It's an existing "bug" though (it's not an error, just a warning), this just added tests that show these warnings.

@agilgur5 agilgur5 mentioned this pull request Nov 24, 2020
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants