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

feat!: typescript configuration and jest v29 upgrade #429

Merged
merged 130 commits into from
Apr 19, 2024
Merged

feat!: typescript configuration and jest v29 upgrade #429

merged 130 commits into from
Apr 19, 2024

Conversation

adamstankiewicz
Copy link
Member

No description provided.

adamstankiewicz and others added 3 commits April 13, 2023 12:12
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
@adamstankiewicz adamstankiewicz changed the title Alpha [do not merge] alpha -> master Jul 28, 2023
renovate bot and others added 26 commits August 7, 2023 14:34
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>
@arbrandes arbrandes self-requested a review March 20, 2024 17:03
Copy link
Contributor

@arbrandes arbrandes left a 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.

Comment on lines 16 to 18
- name: remove tsconfig.json # see issue https://github.com/conventional-changelog/commitlint/issues/3256
run: |
rm tsconfig.json
Copy link
Contributor

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)

example/tsconfig.json Show resolved Hide resolved
README.md Outdated

```Sample json
{
"extends": "@openedx/frontend-build",
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

@arbrandes
Copy link
Contributor

arbrandes commented Mar 20, 2024

Also, we need an interactive rebase on master here to consolidate the commits into a couple of meaningful ones before merging. I suggest:

  • One for all the Typescript stuff
  • One for the Jest Upgrade
  • One for the Renovate upgrades in package-lock

I can try and do that if there are no objections. It will require a force-push to the branch, though, so... heads up.

@arbrandes
Copy link
Contributor

And for the future, alpha should always be treated like an integration branch (because it is). That means that we shouldn't take PRs to it directly. Instead, we should have feature branches based on master (aka PRs) that get rebased often and then octopus-merged into alpha periodically. That way it becomes relatively easy to decide to abandon a feature, or pull another one in.

Also, alpha is meant to be alpha-quality. No guarantees, and expect breaking changes. Only use it temporarily, for testing purposes.

@BilalQamar95
Copy link
Contributor

@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
@abdullahwaheed abdullahwaheed changed the title [do not merge] alpha -> master feat!: typescript configuration and jest v29 upgrade Apr 19, 2024
@abdullahwaheed abdullahwaheed marked this pull request as ready for review April 19, 2024 10:03
@abdullahwaheed
Copy link
Contributor

@arbrandes suggested changes are done, could you please review it again

@arbrandes
Copy link
Contributor

Taking another look!

@arbrandes arbrandes merged commit 91c6ce4 into master Apr 19, 2024
5 of 6 checks passed
@openedx-semantic-release-bot

🎉 This PR is included in version 14.0.0 🎉

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.

10 participants