-
Notifications
You must be signed in to change notification settings - Fork 2k
BSKY-1042: site-spec integration for AI site builder #105543
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
base: trunk
Are you sure you want to change the base?
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
@aagam-shah @dereksmart just to clarify this piece is the site spec lib only, so auth is still not addressed. But I believe it can be a base to turn into an app where we can add the auth layer. |
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.
Code reviewed and tested. I have left some minor comments, plus we want to use the latest version of the site-spec bundle.
Approving as I don't see any major change required.
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 is causing the default root http://calypso.localhost:3000/setup/ai-site-builder/ start at /site-spec, whether it's enabled in config or not. When it's not, it just lands on a white page.
The entrypoints in prod go to http://calypso.localhost:3000/setup/ai-site-builder/ and would be broken.
package.json
Outdated
@@ -108,10 +108,10 @@ | |||
"reformat-files": "./node_modules/.bin/prettier --ignore-path .eslintignore --write \"**/*.{js,jsx,json,ts,tsx}\"", | |||
"search:storybook:start": "echo 'Deprecated, run `yarn workspace @automattic/search run storybook:start` instead'", | |||
"site-admin:storybook:start": "yarn workspace @automattic/site-admin run storybook:start", | |||
"start": "npx check-node-version --package && node bin/welcome.js && yarn run build && yarn run start-build", | |||
"start": "npx check-node-version --package && node --max-old-space-size=8192 bin/welcome.js && yarn run build && yarn run start-build", |
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.
I am very not sure about updating the whole project's start
script. Why is this needed?
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.
You're very right to be unsure about this being for the whole project. Thanks for calling this out.
And no, we shouldn’t force this in the project-wide start - prod/CI already run with higher limits (e.g. TC is set to 16 GB), and hard-coding in the project could unintended cap those. The flag was only to unblock a local js heap out of memory for the welcome.js.
I'm reverting this change in this PR and we can raise a follow-up PR adding a start with higher memory/or per environment memory set.
@@ -174,6 +174,7 @@ | |||
"signup/social-first": true, | |||
"site-indicator": true, | |||
"site-profiler/metrics": true, | |||
"site-spec": true, |
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.
we can enable this in stage.json too (easier to test internally)
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.
Thanks for taking the time to review this, @dereksmart ! |
@ariskataoka can we please not make it the default starting point for http://calypso.localhost:3000/setup/ai-site-builder/ in general? It would be great to have this in staging for internal folks to test it easily without also breaking the current site creation flows. Until we are ready to get this in a production setting, I think the standalone spec should only be accessible with a direct link to it. |
Description
Part of # https://linear.app/a8c/issue/BSKY-1041/integrate-site-spec-in-wordpresscom-environments
This pull request introduces a new system for integrating the SiteSpec lib into Calypso, including configuration, script/CSS loading utils, and a "Site Step" in the AI Site Builder flow that leverages SiteSpec as a test. The changes are grouped into three main areas: feature integration, configuration and env support, and utility/testing infrastructure.
This PR depends on wordpress.com PR 191598.
Why are these changes being made?
We need to replace the current AI site builder with site-spec, also bringing over the ability to build the site specification within Calypso.
Note: This PR does not remove the existing site builder, it just adds a new way to do that. Let's remove the existing one in a separate PR.
Testing Instructions
calypso.mp4
Pre-merge Checklist