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

Support array of paths for moduleNameMapper aliases #9465

Merged
merged 7 commits into from
Feb 2, 2020
Merged

Support array of paths for moduleNameMapper aliases #9465

merged 7 commits into from
Feb 2, 2020

Conversation

OrkhanAlikhanov
Copy link
Contributor

@OrkhanAlikhanov OrkhanAlikhanov commented Jan 24, 2020

Summary

Fixes #8461

Test plan

I've added test cases let me know if I should add more.

I have not updated CHANGELOG.md eventhough PR template suggests it, not sure if I should do it or it's autodone

@facebook-github-bot
Copy link
Contributor

Hi OrkhanAlikhanov! Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@OrkhanAlikhanov
Copy link
Contributor Author

@facebook-github-bot I signed

@codecov-io
Copy link

codecov-io commented Jan 24, 2020

Codecov Report

Merging #9465 into master will decrease coverage by <.01%.
The diff coverage is 57.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9465      +/-   ##
==========================================
- Coverage      65%   64.99%   -0.01%     
==========================================
  Files         283      283              
  Lines       12131    12142      +11     
  Branches     3002     3006       +4     
==========================================
+ Hits         7886     7892       +6     
- Misses       3604     3607       +3     
- Partials      641      643       +2
Impacted Files Coverage Δ
packages/jest-config/src/Descriptions.ts 100% <ø> (ø) ⬆️
packages/jest-resolve/src/index.ts 45.94% <57.14%> (+0.69%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ffdaa75...5815c08. Read the comment docs.

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@OrkhanAlikhanov
Copy link
Contributor Author

Coverage report shows that packages/jest-resolve/src/index.ts some parts are not covered. e2e tests should cover them actually 🤔. I have no idea how I should cover them in packages/jest-resolve/tests

@SimenB
Copy link
Member

SimenB commented Jan 24, 2020

I have not updated CHANGELOG.md eventhough PR template suggests it, not sure if I should do it or it's autodone

It's manual, please update it 🙂

Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

Really solid work, awesome @OrkhanAlikhanov!
Always nice to see that even the low level parts of Jest can be flexible and approachable.

Coverage report shows that packages/jest-resolve/src/index.ts some parts are not covered. e2e tests should cover them actually

coverage does not track e2e tests because of the child processes (that's why our overall coverage metric looks so bad), but don't worry it's not a problem :)

CHANGELOG.md Outdated
@@ -2,6 +2,8 @@

### Features

- `[jest-resolve]` Support array of paths for `moduleNameMapper` aliases ([#9465](https://github.com/facebook/jest/pull/9465))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd give this a [*] scope perhaps, since it's also e.g. config

@@ -377,8 +377,8 @@ export const options = {
moduleNameMapper: {
description:
'A JSON string with a map from regular expressions to ' +
'module names that allow to stub out resources, like images or ' +
'styles with a single module',
'module names or an array of module names that allow to stub ' +
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'module names or an array of module names that allow to stub ' +
'module names or to arrays of module names that allow to stub ' +

(as in 'map from regular expressions to [...] arrays of module names')

Copy link
Contributor

Choose a reason for hiding this comment

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

(also in other file)

packages/jest-resolve/src/index.ts Outdated Show resolved Hide resolved
@OrkhanAlikhanov
Copy link
Contributor Author

@jeysal Hey! Thank you for the in depth review! (also for appreciating my work 🙃) I added 2 more commits also rebased + force pushed.

@OrkhanAlikhanov
Copy link
Contributor Author

I force pushed again because I missed bracket misalignment due to my vscode differ having Ignore Trim Whitespace option turned on.

Even if we add prettyFormat, it is only going to replace JSON.stringify as misalignment issue is due to the string being nested in below code as a part of json-like string to create neat json structured error message

@OrkhanAlikhanov
Copy link
Contributor Author

Friendly ping

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Could you update the documentation as well?

packages/jest-cli/src/cli/args.ts Show resolved Hide resolved
)
: (moduleName: string) => moduleName;

const possibleModuleNames = Array.isArray(mappedModuleName)
Copy link
Member

Choose a reason for hiding this comment

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

Can we make it into an array in normalize?

Thinking about it, that might be a breaking change... Hmm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When an error happens we need to properly format error message based on whether it was set as an array or string in configuration. If we normalize it, we will probably lose that information and not be able to properly format error message

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't be too bad to show [path] even though the user configured path I guess, but probably not worth it also given the breaking change risk. I'd be happy as is - however one thing I do want to verify when I have some time is whether the extra work of Array.isArray, wrapping [mappedModuleName] etc. has a perf impact. I doubt it's significant, but jest-resolve is quite a hot path so better safe than sorry

Copy link
Member

Choose a reason for hiding this comment

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

could normalize to an array in the constructor

Copy link
Contributor Author

@OrkhanAlikhanov OrkhanAlikhanov Jan 30, 2020

Choose a reason for hiding this comment

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

@SimenB Not sure I understood that. Is there anything for me to do apart from updating docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jeysal Aren't resolved module paths cached?

Copy link
Member

Choose a reason for hiding this comment

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

this._possibleModuleNames = Array.isArray(mappedModuleName) ? mappedModuleName : [mappedModuleName];

in the constructor of this class

Copy link
Contributor

Choose a reason for hiding this comment

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

@SimenB The problem is we need both the original config entry for the error message and the array one to iterate over here. As long as we don't want to compromise on the correctness of the error message I think this is as good as it gets 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

@jeysal Aren't resolved module paths cached?

Even with caching often running a single test will need to resolve a large dependency tree, just want to be on the safe side as far as perf goes 😄

Copy link
Member

Choose a reason for hiding this comment

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

the error message can be passed the original config, but in the happy case it won't have to check if array or not

Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

I'd be happy with this as is, but would like to do some simple benchmarking before merge

)
: (moduleName: string) => moduleName;

const possibleModuleNames = Array.isArray(mappedModuleName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't be too bad to show [path] even though the user configured path I guess, but probably not worth it also given the breaking change risk. I'd be happy as is - however one thing I do want to verify when I have some time is whether the extra work of Array.isArray, wrapping [mappedModuleName] etc. has a perf impact. I doubt it's significant, but jest-resolve is quite a hot path so better safe than sorry

Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

Can't find a performance impact - no more open points from my side :)
Thanks a lot @OrkhanAlikhanov!

@SimenB
Copy link
Member

SimenB commented Feb 2, 2020

Just missing documentation update and I'm happy as well 👍

@OrkhanAlikhanov
Copy link
Contributor Author

I updated docs + rebased and force pushed :)

@SimenB SimenB merged commit c9127bf into jestjs:master Feb 2, 2020
@OrkhanAlikhanov
Copy link
Contributor Author

Thank you guys! It was pleasure collaborating with you on this feature! 🚀

@jeysal
Copy link
Contributor

jeysal commented Feb 2, 2020

We must thank you! ♥️ This is solid work, don't hesitate to contribute more if you want :)

@MrBlenny
Copy link

I'm keen to use this feature but it looks like it isnt part of 25.1.0. Would be great to see a release.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Array of paths for moduleNameMapper aliases
6 participants