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

Bw/alpha bump #430

Merged
merged 3 commits into from
Aug 7, 2023
Merged

Bw/alpha bump #430

merged 3 commits into from
Aug 7, 2023

Conversation

muselesscreator
Copy link
Contributor

Rebase to master and add transform to allow ts-jest access in CI.

@muselesscreator muselesscreator requested review from adamstankiewicz and Mashal-m and removed request for Mashal-m August 1, 2023 17:38
@muselesscreator muselesscreator force-pushed the bw/alpha_bump branch 16 times, most recently from bd447fb to cb8fc76 Compare August 4, 2023 03:24
config/jest.config.js Outdated Show resolved Hide resolved
@@ -40,6 +39,6 @@ module.exports = {
configFile: presets.babel.resolvedFilepath,
},
],
...tsjPreset.transform,
'^.+\\.[tj]sx?$': path.resolve(__dirname, '../node_modules/.bin/ts-jest'),
Copy link
Contributor

Choose a reason for hiding this comment

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

[tj]sx also matches '.jsx' (which the above babel-jest transform covers). Is that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hrm... fwiw, this came from @adamstankiewicz, and I believe from an online source before that, so not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

does seem like it would probably be equivalent with just the t in there, though also would be safe this way in case someone removes the earlier transform later?

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't been able to find a documentation on how jest handles transformation where a file is matched by multiple transformers (does it use the first matched transformer, or all of them?). My main concerns here are:
A) Even if this setup is working now, it may be a quirk of implementation that may change later.
B) Client repos may add yet more overlapping transformers to this config, that could interact in unpredictable ways.

Copy link
Member

Choose a reason for hiding this comment

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

The ^.+\\.[tj]sx?$ regex is the same key as used in tsjPreset.transform; only the key's value is different in this change. It's preferable to keep both [tj] such that files that contain TypeScript without the .tsx? extension still get transformed by ts-jest. The ts-jest source code does appear to handle jsx? extensions.

(does it use the first matched transformer, or all of them?)

It uses all of the configured transformers matching the specified file extension(s), processed in the order that they are listed.

Client repos may add yet more overlapping transformers to this config, that could interact in unpredictable ways.

I'm not too concerned about this. Some client repos already customize the jest.config.js. If they do so by extending the default jest.config.js provided by frontend-build versus overriding its default behavior by providing the 2nd arg to createConfig, their changes are deep merged with the default config (additive to transforms). However, if they use custom manipulations (i.e., "Method 2" in README), they could run the risk of removing the default transforms by completely replacing config.transforms. That said, I feel this is a similar risk to pretty much all the other config properties, IMHO.

@muselesscreator muselesscreator merged commit efed83d into alpha Aug 7, 2023
@muselesscreator muselesscreator deleted the bw/alpha_bump branch August 7, 2023 18:10
@edx-semantic-release
Copy link

🎉 This PR is included in version 12.9.0-alpha.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@openedx-semantic-release-bot

🎉 This PR is included in version 12.10.0-alpha.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@openedx-semantic-release-bot

🎉 This PR is included in version 13.1.0-alpha.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@openedx-semantic-release-bot

🎉 This PR is included in version 15.0.0-alpha.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants