-
Notifications
You must be signed in to change notification settings - Fork 33
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
feat!: typescript configuration and jest v29 upgrade #429
Conversation
sync: master to alpha
build: Add .ts/.tsx extensions to webpack resolver chore: Add ending newline to Image.tsx build: Use typescript config from @edx/eslint-config chore: formatting
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…to v3 (#339) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
… to v2 (#310) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
sync: master to alpha
BREAKING CHANGE: Updating jest from v26 to v29 changes the default snapshotFormat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No objections in principle, but I feel we should answer a couple of questions.
.github/workflows/commitlint.yml
Outdated
- name: remove tsconfig.json # see issue https://github.com/conventional-changelog/commitlint/issues/3256 | ||
run: | | ||
rm tsconfig.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're not going to need this, going forward. See this comment on a Paragon PR: openedx/paragon#2867 (comment)
README.md
Outdated
|
||
```Sample json | ||
{ | ||
"extends": "@openedx/frontend-build", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we're actually extending @edx/typescript-config
in the tsconfig in this repo. Should we not recommend doing the same elsewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it might be good to recommend setting outDir
, because I found in openedx/paragon#3016 that if you extend @edx/typescript-config
but don't set outDir
, when you run tsc
the files will get output to ./node_modules/@edx/typescript-config/dist/
- which is not that different from sending them to /dev/null
. Most people won't be running tsc
(rather webpack, babel, etc.) but if they do this heads up will save them some time trying to figure out why the output is not showing up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we're actually extending @edx/typescript-config in the tsconfig in this repo. Should we not recommend doing the same elsewhere?
@arbrandes I believe the original intent for recommending @openedx/frontend-build
here was that there might be MFE-specific additions/updates we want in the base TS config available to MFEs that don't make sense in the upstream @edx/typescript-config
(which, in theory, could be used for more beyond just MFEs).
For example, the base ESLint config provided to MFEs via frontend-build extends @edx/eslint-config
, but slightly differs. This approach to extend @openedx/frontend-build
was largely following that paradigm.
That said, if supporting MFE-only TypeScript config isn't necessary (which is probably isn't; I don't really see legacy UI repos like edx-platform adopting TypeScript anytime soon), I agree recommending to extend @edx/typescript-config
directly makes more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I've always wondered why typescript-config
and eslint-config
existed. Personally, I'd prefer to just consolidate them into frontend-build
or frontend-platform
.
Also, we need an interactive rebase on master here to consolidate the commits into a couple of meaningful ones before merging. I suggest:
I can try and do that if there are no objections. It will require a force-push to the branch, though, so... heads up. |
And for the future, Also, |
…to bilalqamar95/alpha-eslint-update
feat: Updated eslint-config major version
sync: master to alpha
@arbrandes With alpha synced with master, I believe this PR is ready to be reviewed/merged, it would help unblock jest v29 upgrade |
* refactor: updated Readme * refactor: updated commitlint workflow file
@arbrandes suggested changes are done, could you please review it again |
Taking another look! |
🎉 This PR is included in version 14.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 15.0.0-alpha.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
No description provided.