Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
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 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Starter Plugin plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Classic Theme helper plugin plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
b5f288c to
fd3a6ac
Compare
Code Coverage SummaryCannot generate coverage summary while tests are failing. 🤐 Please fix the tests, or re-run the Code coverage job if it was something being flaky. |
486cf38 to
e93e444
Compare
643217f to
93a48e4
Compare
e93e444 to
48b4c75
Compare
anomiex
left a comment
There was a problem hiding this comment.
Looks reasonable so far.
I wonder if @manzoorwanijk has any better ideas about the inline comments, he knows TypeScript stuff a lot better than I do.
|
|
||
| return ( | ||
| <div ref={ uplotContainer } className="boost-uplot-container"> | ||
| { /* @ts-expect-error JSX element type 'UplotReact' does not have any construct or call signatures.ts(2604) */ } |
There was a problem hiding this comment.
As far as I can tell, there's either some sort of weirdness when a package's default export is a function that gets TypeScript confused, or there's some sort of confusing mismatch between what the .js actually has and what the .d.ts says when the package's default export is a function. I haven't been able to find a really clear explanation.
Either way, this seems like a reasonable workaround to me. 🤷
There was a problem hiding this comment.
Ended up with an interesting conflict where jetpack build plugins/jetpack would fail due to no TS error being thrown, but required for jetpack build plugins/jetpack --production when the TS errors are thrown.
I resorted to typecasting with as unknown as ... to manually specify the component types for UplotReact and ReactSlider.
|
|
||
| return ( | ||
| <div className={ componentClassName } data-testid="number-slider"> | ||
| { /* @ts-expect-error JSX element type 'ReactSlider' does not have any construct or call signatures.ts(2604) */ } |
projects/js-packages/components/components/pricing-card/test/component.tsx
Outdated
Show resolved
Hide resolved
|
Using this tsconfig should fix those errors {
"extends": "jetpack-js-tools/tsconfig.tsc.json",
"include": [ "./index.ts", "./components", "./lib", "./tools" ],
"compilerOptions": {
"module": "esnext",
"moduleResolution": "bundler",
// ... Rest of the config
}
} |
|
As rightly pointed out by @anomiex, if you wish to include test files in tsconfig, then you will need to have a separate tsconfig for build. |
878df52 to
60f0892
Compare
48b4c75 to
9ff1e56
Compare
Using that tsconfig will fail the test we have that complains about trying to set "module" or "moduleResolution". 😀 After getting complaints about how "bundler" is infectious and seeing that TypeScript says libraries should use "nodenext" for best compatibility, we set up the current structure with three base tsconfig files and tests that reject use of "module", "moduleResolution", or "noEmit" elsewhere.
|
|
Instead of having the bundling in that components package, should we instead bundle it via a PHP package like That way, we won't need to change anything with the components package as it's used in many different ways that may break. For example, the weird imports like these can be tedious and can result in duplicated code in bundles |
The package still needs to be usable to build against. And not every reuser would want to (or be able to) externalize the dependency and use a PHP package.
That's definitely something that needs to be fixed before this PR can work. See some prior discussion at #41524 (comment) for a few more details. |
9ff1e56 to
1ae73fb
Compare
anomiex
left a comment
There was a problem hiding this comment.
Looks good to me from a Garage perspective, assuming we're ok with losing TypeScript type checking of the various test files. The right files are being produced in the build artifact, no unexpected changes.
anomiex
left a comment
There was a problem hiding this comment.
Now that I think of it, we should probably update
https://github.com/Automattic/jetpack/blob/1571c9f4d847cbe757727d73b2b6f747ecedd345/projects/js-packages/components/package.json#L87-L88
to be something like
"build": "pnpm run clean && pnpm run compile-ts",
"clean": "rm -rf build/",
"compile-ts": "tsc --pretty",Co-authored-by: Brad Jorsch <anomiex@users.noreply.github.com>
Co-authored-by: Brad Jorsch <anomiex@users.noreply.github.com>
Co-authored-by: Brad Jorsch <anomiex@users.noreply.github.com>
510a836 to
9d02ba5
Compare
|
@anomiex Applied all of your suggestions, builds and tests continue to pass, and TS hints/lints appear to work nicely for package consumers 👍 |
Proposed changes:
tsconfig.tsc.jsonbase with customcompilerOptions.Other information:
Jetpack product discussion
p1738271950791069-slack-CDLH4C1UZ
Does this pull request change what data or activity we track or use?
No
Testing instructions:
jetpack build js-packages/components -vandjetpack build js-packages/components --production -vrun successfully.jetpack build plugins/jetpack -vandjetpack build plugins/jetpack --production -vrun successfully.git status).builddirectory contents.