-
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 JS Package: Remove Build #40262
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! |
e5e11bb
to
3bd6052
Compare
3bd6052
to
8870b01
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.
🤷 This improves on the current situation. OTOH, it makes the package less usable for people outside the monorepo (cf. #39532). I don't know whether that's a goal for this package.
Co-authored-by: Brad Jorsch <[email protected]>
@anomiex Right now, the Scan/Security team's intention is primarily to provide a means of sharing code across the main Jetpack plugin and Jetpack Protect. Although if the package was more easily consumable outside of the monorepo, we could use it in Calypso or other places in the future, which sounds nice. I suppose the other direction would be to build the build, and distribute the JS. I'm not sure exactly what that would mean in terms of the resulting bundling/affects on deps and peer deps, if anything. I can explore that more and draft an alternate PR testing out that approach. 👍 |
Within the monorepo it shouldn't change anything, since stuff within the monorepo should continue to use the "jetpack:src" entries. Externally it'd bring back a I don't see any relevant dependency changes in/since #39754, so that should be fine. But you'll have to resolve whatever errors led to 377e873 in a different manner. If the problem was stuff like
from jest tests then this might help: patchdiff --git a/tools/js-tools/package.json b/tools/js-tools/package.json
index 784f44cf8d..43c9ca9d81 100644
--- a/tools/js-tools/package.json
+++ b/tools/js-tools/package.json
@@ -56,6 +56,7 @@
"sort-package-json": "1.50.0",
"svelte": "4.2.19",
"svelte-eslint-parser": "0.39.2",
+ "ts-jest-resolver": "2.0.1",
"typescript": "5.0.4",
"yaml": "2.2.2"
}
diff --git a/tools/js-tools/jest/jest-resolver.js b/tools/js-tools/jest/jest-resolver.js
index 597c41156b..03353b69e7 100644
--- a/tools/js-tools/jest/jest-resolver.js
+++ b/tools/js-tools/jest/jest-resolver.js
@@ -1,3 +1,5 @@
+const tsJestResolver = require( 'ts-jest-resolver' );
+
// Some packages assume that a "browser" environment is esm or otherwise break in node.
// List them here and the resolver will adjust the conditions to resolve them as "node" instead.
// cf. https://github.com/microsoft/accessibility-insights-web/pull/5421#issuecomment-1109168149
@@ -20,7 +22,7 @@ module.exports = ( path, options ) => {
conditions.add( 'node' );
}
- return options.defaultResolver( path, {
+ return tsJestResolver( path, {
...options,
basedir,
conditions, |
Closing this in favor of #40299 |
Proposed changes:
package.json
file.jetpack:src
fromexports
, sincedefault
is now the same./src/index.ts
value.composer.json
.Other information:
Jetpack product discussion
p1732032090059999-slack-CDLH4C1UZ
peb6dq-36i-p2
Does this pull request change what data or activity we track or use?
No
Testing instructions:
cd projects/js-packages/storybook && npm run storybook:dev
http://localhost:50240/?path=/docs/js-packages-components-threat-fixer-button--docs