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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
deacfed
fix: remove tsconfig for commitlint job
muselesscreator Aug 7, 2023
3b8f66a
fix: fis jest config for ts-jest access
muselesscreator Aug 7, 2023
a0f24df
fix: remove extraneous dependency
muselesscreator Aug 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion .github/workflows/commitlint.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,14 @@ on:

jobs:
commitlint:
uses: openedx/.github/.github/workflows/commitlint.yml@master
runs-on: ubuntu-20.04
steps:
- name: Check out repository
uses: actions/checkout@v3
with:
fetch-depth: 0
- name: remove tsconfig.json # see issue https://github.com/conventional-changelog/commitlint/issues/3256
run: |
rm tsconfig.json
muselesscreator marked this conversation as resolved.
Show resolved Hide resolved
- name: Check commits
uses: wagoid/commitlint-github-action@v5
3 changes: 1 addition & 2 deletions config/jest.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
const path = require('path');
const fs = require('fs');
const { jsWithTs: tsjPreset } = require('ts-jest/presets');

const presets = require('../lib/presets');

Expand Down Expand Up @@ -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.

},
};
1 change: 0 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@
"source-map-loader": "^4.0.1",
"ts-jest": "^26.5.0",
"style-loader": "3.3.3",
"ts-jest": "^26.5.0",
"typescript": "^4.9.4",
"url-loader": "4.1.1",
"webpack": "5.88.2",
Expand Down