-
Notifications
You must be signed in to change notification settings - Fork 2k
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
FSE: Integrate wp-env environment #39000
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~128111 bytes removed 📉 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~34747 bytes removed 📉 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. Moment.js Locales (~3979 bytes removed 📉 [gzipped])
Locale data for moment.js. Unless you are upgrading the moment.js library, changes in these chunks are suspicious. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
apparently it is impossible to entirely override the ports: docker/compose#2260. Docker will actually merge those ports |
Solved by adding the port as an environment variable in the run script :) |
apps/full-site-editing/bin/docker-compose.override.yml.template
Outdated
Show resolved
Hide resolved
1087844
to
a82edbf
Compare
The failing e2e tests don't appear to be related to our additions here. I do see this error:
|
Added @Automattic/luna in case folks have opinions on some of this stuff. After all, we'll all be working in this environment :) |
After running the
Visiting
I'll leave it there for now as I go to debug what went wrong and see if I can help with a fix. Looks like the FSE plugin didn't get built properly. |
@mattwiebe I've seen that error before while running the plugin on wpcom, and usually it is fixed by re-building the plugin. It sounds like we may need to run the fse build step before trying to load it in the environment. |
cd9beec
to
579eef3
Compare
@mattwiebe I fixed it by making the script also run the build first. Try:
|
.eslintrc.js
Outdated
@@ -33,6 +33,14 @@ module.exports = { | |||
step: false, | |||
}, | |||
}, | |||
{ | |||
files: [ 'apps/full-site-editing/**/tests/**' ], |
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.
TODO: improve this so that it matches .test.js and .spec.js files
Okay, passed the first step. Had to run Also, the first time I ran it and then logged into Then I started to try to run the tests. ✅ Here's the
I'll try to dig in more. Great work getting us this far! 🎉 |
e4bceed
to
8a9699a
Compare
8a9699a
to
1bb3d76
Compare
1bb3d76
to
6778e29
Compare
Update: see the new PR description. I've removed all the testing related code from this PR and narrowed it down to just adding @wordpress/env. This should make it easier for me to iterate on the individual pieces of the puzzle :) In particular, #39326 should be completely ready to go. Still doing some work on this PR and the e2e one, but the phpunit one is completely blocked at the moment. |
6cbf562
to
63f603e
Compare
@@ -0,0 +1,6 @@ | |||
{ | |||
"plugins": [ "./full-site-editing-plugin", "../../../gutenberg" ], |
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.
What do you think about including Jetpack and wpcomsh here so it's closer to Atomic env?
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 could definitely do that. Do you think it could be a follow up?
One of the reasons that it might not be nice to do this is that every plugin and theme here is the development version. So I'd have to do npm i
and npm run build
for every plugin so that it loads correctly. And then I would have to make sure that I do git pull
regularly. I could see that being a pain with a lot of plugins.
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.
Do you think it could be a follow up?
Sure, that's fine with me.
The current testing steps work fine for me up to a certain point. 🚀The site's front end and wp-admin load fine. When I try to open the editor though I'm running into this fatal, not sure how it's related given that this is a core WP function.
I'm guessing this is also the culprit behind my errors in e2e testing PR. Also, what do you think about adding |
Honestly, I wanted to do that, but I was thinking that it'd be really verbose to run it with npm:
at which point referencing the script directly seems to make more sense. Plus, the script is written such that expects to start in the wp-calypso root, and not in the FSE app folder. (Though I could improve that :)) Plus, this is just for setting up the environment, so it should be a one-time thing. Once everything is working, you should be able to run
My only guess is that it's a bug in Gutenberg?? The WordPress version is the latest production version (I think 5.3), not the dev version. Could try re-building the Gutenberg plugin to see if that works since the error is thrown in |
Oh true, for a moment I forgot how much incantations it takes now. 😄 |
Because it is not used for anything :)
45f3762
to
e2a93d2
Compare
I tested this and everything works fine except loading the Gutenberg editor which results in the following fatal:
I tracked it down, and the problem seems to be stemming from the fact that my env is running WP 5.2.2, and required I suspected that docker might be using some of my old WP images, but I purged them all and made sure that the script downloads a new one, and it still ended up being 5.2.2. Having said that, this problem is not caused by this PR and it's something that needs to be resolved in the WP docker image or |
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.
LGTM!
Originally, I covered several more things in this PR which I have split out into the following:
This PR sets up @wordpress/env as a Docker environment for running the plugin locally (and hopefully, in the future, in CI).
As part of this effort, I have created an opinionated "setup-env" script which sets up the repositories a developer needs for working with the FSE plugin locally.
localhost:4013
Changes proposed in this Pull Request
.wp-env.json
file to specify dependencies of the project.Test Install Step
master
branch, and build everything:nvm use && npm ci && npm run build
../apps/full-site-editing/bin/setup-env.sh
. Once it does everything, it will ask you if you want to run the development version of wp-env. Choose yes since the changes we require have not been published to npm yet.localhost:4013
. (Note: I have experienced issues with my browser caching a redirect between different ports, so you may need to try different browsers and/or clear the cache forlocalhost
).To do: