Fix options.moduleNameMapper override order with preset (#3565) #3689

markwhitfeld · 2017-05-30T12:10:50Z

Summary

Adjusted the order so that the options mappings are checked first and then the preset mappings are checked.
The preset mappings can be overridden by the options mappings.
Fixes #3565

Test plan

Modified existing test and added a new test.
Tests passed after making the necessary code changes. TDD FTW ;) !

markwhitfeld · 2017-05-30T12:57:44Z

I think that some "prettier" or ESLint checks failed on CircleCi (hard to tell from the output):

/home/ubuntu/jest/packages/jest-config/src/tests/normalize-test.js
842:27 error There should be no space after '{' object-curly-spacing
842:38 error Expected object keys to be in ascending order. 'c' should be before 'e' sort-keys
842:47 error Expected object keys to be in ascending order. 'b' should be before 'c' sort-keys
842:56 error Expected object keys to be in ascending order. 'a' should be before 'b' sort-keys
842:64 error There should be no space before '}' object-curly-spacing

The first an last errors are just some whitespace that my vs code formatting added. I'll fix that.
The middle three are a bit of a problem though. I deliberately ordered my object keys non-alphabetically to make for a better test. I don't believe that this should change. I will add a prettier ignore directive to the code.

(ESLint and prettier do not work after forking and following all the contribution instructions, so a client side check is difficult. Not a great experience I must say... may have just been in a broken state at the point when I forked)

codecov-io · 2017-05-30T14:15:12Z

Codecov Report

Merging #3689 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3689   +/-   ##
=======================================
  Coverage   58.08%   58.08%           
=======================================
  Files         195      195           
  Lines        6751     6751           
  Branches        6        6           
=======================================
  Hits         3921     3921           
  Misses       2827     2827           
  Partials        3        3

Impacted Files	Coverage Δ
packages/jest-config/src/normalize.js	`85.38% <ø> (ø)`	⬆️

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 88e2ee8...b932948. Read the comment docs.

markwhitfeld · 2017-05-30T19:49:00Z

It would be great if this could be part of the next release. I am assuming you are doing trunk based dev so I created this pull request for master.

treveshan · 2017-05-31T09:24:14Z

wow finally someone solved this!!! #Legend

treveshan

Looks good to me

markwhitfeld · 2017-05-31T09:30:22Z

@thymikee are you able to approve this and merge it?
This should fix an issue in your jest-preset-angular repo too: thymikee/jest-preset-angular#37

thymikee · 2017-06-01T18:07:25Z

Maybe a little background first. Presets may have some very loosely declared mappings, e.g.:

preset:

"moduleNameMapper": {
  "(.*)": "<rootDir>/src/$1"
},

user config:

"moduleNameMapper": {
  "(.*)": "$1",
  "/my-module/(.*)": "<rootDir>/abc/$1"
},

This mapping ("(.*)": "<rootDir>/src/$1") can be overwritten with user config, but as soon as it gets evaluated by jest-resolve, because it's so loose, other mappings (like "/my-module/(.*)": "<rootDir>/abc/$1" are not taken into consideration.

What this PR does is that it lets in a bit hacky way overcome this issue by merging moduleNameMapper from user config first just to get the right order of mappings.

What do you think @cpojer?

markwhitfeld · 2017-06-09T06:32:12Z

@thymikee and @cpojer
Any idea on when this might be merged?
My team is itching to start using jest but this is a blocker for us.

thymikee · 2017-06-09T06:34:40Z

By the time it's not merged (I'm not sure if it will be), you can temporarily copy the preset configuration and adjust it the way you like. It's not a blocker.

cpojer · 2017-06-09T15:04:59Z

packages/jest-config/src/__tests__/normalize-test.js

+
+  test('merges with options and moduleNameMapper preset is overridden by options', () => {
+    /* eslint-disable sort-keys */
+    // prettier-ignore


Can you please enable prettier here and enable sort keys?

Thanks for the review. See my comment above about the sort-keys rule.
I really think that a test for the correct handling of the order precedence of user and preset config should not only give a scenario that could imply that alphabetical order is the precedence of choice. Providing the object properties out of order makes the test more explicit and more resilient in my opinion.
I'm happy to change my test to support the rule and remove the disable if you prefer the following of the style rules over a more resilient test. Let me know ;)

I was just thinking that I could alternatively retain the resilience of the test just by changing semantics. Instead of using the object initializer I could create an empty object and then assign each property in separate statements. Please let me know what you would prefer. Thanks!

Yeah that sounds reasonable.

Great...
Which one? The last suggestion? 😄

cpojer · 2017-06-09T15:06:01Z

I don't really have super strong feelings about this, if you fix up the test code to not opt-out of the code standards from this repo, I'm fine merging it.

markwhitfeld · 2017-06-13T08:00:47Z

The Circleci build failed on my latest commit due to a failure of the 'yarn run danger' step.
I can't see how that is related to my change. Maybe it is a current issue on master or an intermittent failure.
@cpojer could you kick off a rebuild?

thymikee · 2017-06-13T08:07:55Z

This also happened on another PR. Cc @orta

orta · 2017-06-13T10:58:55Z

Weird, that looks like a bug on GitHub's end wrt their API - Danger's making an API call to an open repo, doesn't look like it's being rate-limited either. So I wouldn't expect this to ever fail.

markwhitfeld · 2017-06-13T13:50:24Z

@cpojer looks like that may be an intermittent isue in the one build step. Would you be able to kick off a rebuild, or should I do a small superfluous check-in to get it rebuilding again?

markwhitfeld · 2017-06-19T11:16:00Z

@orta @cpojer @thymikee Does any of you know how I can get a build kicked off again for this merge request?

markwhitfeld · 2017-06-20T10:22:28Z

Thanks @thymikee for kicking off a rebuild.
Unfortunately the build issue still persists. It seems to be a 401 Unauthorized error being returned by github in the 'danger' build step.
Does anyone have an idea of how we could progress this issue?
Do I need to do anything my side?

thymikee · 2017-06-20T10:26:41Z

All tests pass, danger token issue is known and there's nothing you can do but wait for your PR to be merged. @cpojer is on his vacation now, so it may take a while.

markwhitfeld · 2017-06-22T05:57:06Z

Hope you have a great time away @cpojer!
Is there anyone else able to help with this pull request?
@aaronabramov maybe?

michahell · 2017-06-22T12:12:07Z

This is a really annoying issue with jest. Is there a better way to temporarily overcome this than:

manually merging all files from jest-preset-angular so there is only 1 moduleNameMapper config
editing/forking the jest-preset-angular package to change the moduleNameMapper config
commenting out any imports in code under test that are not js|ts|html refs

?

thymikee · 2017-06-22T12:14:47Z

@michahell you can directly copy jest-preset.json to your package.json and remove the mapping as a workaround for now.

cpojer · 2017-06-27T08:11:37Z

This'll need one more rebase, sorry.

markwhitfeld · 2017-06-27T10:34:20Z

Ok, I'll do that now

Adjusted the order so that the options mappings are checked first and then the preset mappings are checked. The preset mappings can be overridden by the options mappings.

markwhitfeld · 2017-06-27T11:34:19Z

Thanks @cpojer!
When is the next planned release?

cpojer · 2017-06-27T11:37:34Z

May 17th, 2019.

michahell · 2017-06-27T15:16:52Z

Thanks guys, awesome! I'll postpone writing tests to May 17th, 2019. don't worry, none will notice 😃 😂

markwhitfeld · 2017-06-27T17:08:58Z

Come on @michahell I think that is taking it a bit too far. May 17th, 2019 is a Friday 🍺 ! You will have to get them done on the 16th 😜

…jestjs#3689) * Fix options.moduleNameMapper override order with preset (jestjs#3565) Adjusted the order so that the options mappings are checked first and then the preset mappings are checked. The preset mappings can be overridden by the options mappings. * Fix prettier linting issues * Fix eslint issues * Workaround in test for eslint sort-keys rule

github-actions · 2021-05-13T12:06:43Z

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.

facebook-github-bot added the CLA Signed ✔️ label May 30, 2017

markwhitfeld mentioned this pull request May 30, 2017

override/add to moduleNameWrapper doesn't work thymikee/jest-preset-angular#37

Closed

treveshan approved these changes May 31, 2017

View reviewed changes

cpojer reviewed Jun 9, 2017

View reviewed changes

Mark.Whitfeld added 4 commits June 27, 2017 12:37

Fix options.moduleNameMapper override order with preset (jestjs#3565)

fd5047b

Adjusted the order so that the options mappings are checked first and then the preset mappings are checked. The preset mappings can be overridden by the options mappings.

Fix prettier linting issues

dceaf61

Fix eslint issues

47e42cd

Workaround in test for eslint sort-keys rule

b932948

markwhitfeld force-pushed the master branch from 6d25256 to b932948 Compare June 27, 2017 11:26

cpojer merged commit 98a6d9d into jestjs:master Jun 27, 2017

michahell mentioned this pull request Jun 27, 2017

I could not use MaterialModule thymikee/jest-preset-angular#52

Closed

thymikee mentioned this pull request Jul 3, 2017

Tighten up module mappers to prevent hard-to-debug bugs thymikee/jest-preset-angular#53

Merged

thymikee mentioned this pull request Feb 9, 2018

Make it possible to merge transform option with preset #5505

Merged

thymikee mentioned this pull request Nov 13, 2019

jest-config: remove needless object spread in merge object #9179

Closed

github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix options.moduleNameMapper override order with preset (#3565) #3689

Fix options.moduleNameMapper override order with preset (#3565) #3689

markwhitfeld commented May 30, 2017 •

edited by thymikee

Loading

markwhitfeld commented May 30, 2017 •

edited

Loading

codecov-io commented May 30, 2017 •

edited

Loading

markwhitfeld commented May 30, 2017

treveshan commented May 31, 2017

treveshan left a comment

markwhitfeld commented May 31, 2017 •

edited

Loading

thymikee commented Jun 1, 2017

markwhitfeld commented Jun 9, 2017

thymikee commented Jun 9, 2017

cpojer Jun 9, 2017

markwhitfeld Jun 9, 2017

markwhitfeld Jun 10, 2017

cpojer Jun 12, 2017

markwhitfeld Jun 12, 2017

cpojer Jun 12, 2017

markwhitfeld Jun 13, 2017

cpojer commented Jun 9, 2017

markwhitfeld commented Jun 13, 2017

thymikee commented Jun 13, 2017

orta commented Jun 13, 2017

markwhitfeld commented Jun 13, 2017

markwhitfeld commented Jun 19, 2017

markwhitfeld commented Jun 20, 2017

thymikee commented Jun 20, 2017

markwhitfeld commented Jun 22, 2017

michahell commented Jun 22, 2017 •

edited

Loading

thymikee commented Jun 22, 2017

cpojer commented Jun 27, 2017

markwhitfeld commented Jun 27, 2017

markwhitfeld commented Jun 27, 2017

cpojer commented Jun 27, 2017

michahell commented Jun 27, 2017

markwhitfeld commented Jun 27, 2017

github-actions bot commented May 13, 2021

Fix options.moduleNameMapper override order with preset (#3565) #3689

Fix options.moduleNameMapper override order with preset (#3565) #3689

Conversation

markwhitfeld commented May 30, 2017 • edited by thymikee Loading

markwhitfeld commented May 30, 2017 • edited Loading

codecov-io commented May 30, 2017 • edited Loading

Codecov Report

markwhitfeld commented May 30, 2017

treveshan commented May 31, 2017

treveshan left a comment

Choose a reason for hiding this comment

markwhitfeld commented May 31, 2017 • edited Loading

thymikee commented Jun 1, 2017

markwhitfeld commented Jun 9, 2017

thymikee commented Jun 9, 2017

cpojer Jun 9, 2017

Choose a reason for hiding this comment

markwhitfeld Jun 9, 2017

Choose a reason for hiding this comment

markwhitfeld Jun 10, 2017

Choose a reason for hiding this comment

cpojer Jun 12, 2017

Choose a reason for hiding this comment

markwhitfeld Jun 12, 2017

Choose a reason for hiding this comment

cpojer Jun 12, 2017

Choose a reason for hiding this comment

markwhitfeld Jun 13, 2017

Choose a reason for hiding this comment

cpojer commented Jun 9, 2017

markwhitfeld commented Jun 13, 2017

thymikee commented Jun 13, 2017

orta commented Jun 13, 2017

markwhitfeld commented Jun 13, 2017

markwhitfeld commented Jun 19, 2017

markwhitfeld commented Jun 20, 2017

thymikee commented Jun 20, 2017

markwhitfeld commented Jun 22, 2017

michahell commented Jun 22, 2017 • edited Loading

thymikee commented Jun 22, 2017

cpojer commented Jun 27, 2017

markwhitfeld commented Jun 27, 2017

markwhitfeld commented Jun 27, 2017

cpojer commented Jun 27, 2017

michahell commented Jun 27, 2017

markwhitfeld commented Jun 27, 2017

github-actions bot commented May 13, 2021

markwhitfeld commented May 30, 2017 •

edited by thymikee

Loading

markwhitfeld commented May 30, 2017 •

edited

Loading

codecov-io commented May 30, 2017 •

edited

Loading

markwhitfeld commented May 31, 2017 •

edited

Loading

michahell commented Jun 22, 2017 •

edited

Loading