-
Notifications
You must be signed in to change notification settings - Fork 795
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
Add TypeScript support #23522
Add TypeScript support #23522
Conversation
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped. Jetpack plugin:
|
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.
This PR seems to be doing too many different things:
- Fixing VSCode's locating of Jetpack components.
- Fixing VSCode's locating of monorepo JS packages, if that's even broken?
- Adding typescript support all over the place.
- Moving code around to create a new component.
I'd like to see a version that does the minimum possible to handle just the first and second bullets (if the second is actually a problem). Then do the others as followups.
As it is, I can't tell which things are here to fix vscode, which are here to try to support typescript, and which are extra stuff that got added in trying to figure out vscode and didn't get backed out if they didn't work.
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.
This does play nice in VS code. It doesn't quite fix the svelte issue with Prettier that we discussed in #20996, but it does make it really easier to navigate between components, nice work!
There are a few failing tests you'll need to address, as you can see below, related to version bumps and changelogs.
I've added @Automattic/jetpack-agora as reviewers for this PR. If we are adding TypeScript support to the Components package and to the Jetpack, it may be good to discuss (here or maybe preferably on +jetpackcrew) whether we should start encouraging / mandating the use of TypeScript in the monorepo from now on, setting the right expectations for everyone contributing, to avoid having a mix in the components from now on, all depending on who writes the first version of the component, and thus making it a bit more difficult for new contributors to jump in and contribute to a specific part of Jetpack. |
Actually all of those things are basically the same problem - i.e. VS Code is not able to locate the imported components. If we try to fix those separately, we may have many test failures thus it will mean we will need to merge PRs that break many things. |
You can see that I mentioned the issues that this PR fixes. ESLint/Prettier issue is another big one that can be handled separately in another PR. |
62cad2e
to
ee8ac6e
Compare
How exactly are adding typescript support for all sorts of things and moving and rewriting I could see that you might need a minimal typescript config file to teach vscode how to find things if that's where it insists on getting its configs from, but it seems to me this PR goes beyond that. |
jetpack-components IMO
Even sounds like a nice plan :-D We agree that TypeScript is something that we should start to encourage to use sooner than later. Being said that and just in case, I'm not the right person to lead this refactoring process but 100more than happy to get 100% involved on it. First action to take is to document the process, some rules, whatever that can make the developers tasks painless as much as possible. |
bae992a
to
2b98c70
Compare
5356fdf
to
7fc0278
Compare
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.
Everything looks good for me! .. Did some tests and it looks fine, it will be a good movement for RNA Components 🤘🏽
Caution: This PR has changes that must be merged to WordPress.com |
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.
Looks ok to me now as far as structure goes. I'll leave it to someone from Agora to do a review of whether the actual typescript stuff is right.
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.
Did some tests at jetpack-components
and everything went well. Let's move forward and we could start to improve the configuration as soon we have more use cases.
@manzoorwanijk fwiw there is a way to dismiss the "requested changes" thing. |
Great news! One last step: head over to your WordPress.com diff, D78670-code, and deploy it. Thank you! |
I think that also requires higher permissions Thank you for doing that for me @brbrr |
r243684-wpcom |
Will it be worth to take this discussion to a p2 post? @retrofox @jeherve @renatoagds @oskosk There is already this p2 post I found p9dueE-3Hs-p2 (by @allilevine) Whenever you plan to work on migrating components to TypeScript, count me in for any help. |
js-packages
,plugins/jetpack
and this can be extended to any part of the codebase.Also,
@types/react
etc.) as dependenciesJetpack product discussion
p9dueE-4E8-p2
Does this pull request change what data or activity we track or use?
No
Testing instructions
jetpack build plugin/jetpack
js-packages/components
See #23889 as an example/POC to use TypeScript.