-
Notifications
You must be signed in to change notification settings - Fork 800
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
Scan Package: Fix Build #40299
Scan Package: Fix Build #40299
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 🤖 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. Follow this PR Review Process:
Still unsure? Reach out in #jetpack-developers for guidance! |
f45ec68
to
aa72eaf
Compare
428ed09
to
ff114a3
Compare
@anomiex What do you think about this alternate to #40262? I had to extend My ultimate hope and dream for the scan package is to have a reusable TS lib that exports types, functions, and translatable strings related to Jetpack Scan features. Initially for use across Jetpack and Jetpack Protect plugins, and then potentially sharing code with Calypso if/as possible and useful. I know the inclusion of Edit: Could we use the declaration-only base to export types, and then have consumers (Jetpack, Jetpack Protect) be responsible for compiling the TS? |
No, if you're using tsc to build then you should be using The reason for that is that If you can't figure the tests out on your own, feel free to push what you have and ask for help. 🙂
You could, but there's not much point IMO. The intent of the declaration-only base is for when you want to use webpack instead of tsc to compile the TS to JS but also want to produce The consumers having to compile the TS themselves is effectively the case now, and if it's the way you want to go with this package then #40262 should be done to clean up. The down side is that it'll be harder to use in external consumers such as Calypso, because they would also have to be configured to compile the TS from this package themselves. |
Thanks again @anomiex, your expertise here is super valuable. I would vote we continue not shipping JS, and have re-opened a PR for that here: #40318 My thought is, we need the consumers/plugins to compile the translations. Otherwise, we could just build and ship JS right from the plugin if it was just types and utils. P2'd this as well: peb6dq-3as-p2 |
Translations are a whole other issue! 😀 For modern-ish plugins on dotorg, translation generally happens via translate.wordpress.org based on what JS is in the actual bundles. Whether that was compiled directly from TS into the bundle or was compiled to JS and then the JS was bundled doesn't make a difference. For other stuff it all depends on their toolchain for doing translations. Which as far as I know there isn't much of a standard around, but I'd be surprised if compiling directly from a TS-package versus using already-compiled JS from a JS-package made much difference. |
ff114a3
to
ae3f123
Compare
@anomiex Thanks again for the guidance! I've updated this PR. Here's the outline:
This will allow us to write TS, export JS, and maintain compatibility with monorepo tooling. I've been using the |
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 reasonable to me, and I see CI is happy.
Following up on #40262, this PR ensures the
js-packages/scan
project actually builds thebuild
folder usingtsc
.Proposed changes:
tsconfig.json
made in https://github.com/Automattic/jetpack/pull/39754/files#diff-1a0db43237e720fa764bf7d3052b6c672d5c26f7c7479154dca7c215f16270fatsc
.ts-jest-resolver
totools/js-tests
, to enable compatibility with the scan package's ".js" imports but ".ts" jetpack source.Other information:
Jetpack product discussion
#40262
Does this pull request change what data or activity we track or use?
No
Testing instructions:
jetpack build js-packages/scan
and validate thebuild
directory is generated.