Skip to content
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

Framework: Replace all usage of wp-scripts env with @wordpress/env. #17871

Closed
wants to merge 4 commits into from

Conversation

epiqueras
Copy link
Contributor

Follows #17724

Description

This PR replaces all usage of wp-scripts env with @wordpress/env.

How has this been tested?

It was verified that all changed commands, including E2E testing and PHP linting still work as expected. The Travis usage also indirectly helped with this.

Types of Changes

Breaking Change: The new env family of scripts has been removed (#17004), use @wordpress/env instead.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@epiqueras epiqueras added Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Build Tooling Issues or PRs related to build tooling labels Oct 9, 2019
@epiqueras epiqueras added this to the Future milestone Oct 9, 2019
@epiqueras epiqueras self-assigned this Oct 9, 2019
@epiqueras epiqueras force-pushed the try/replacing-tests-env branch 7 times, most recently from 93f11e0 to 4a87158 Compare October 9, 2019 23:26
@gziolo
Copy link
Member

gziolo commented Oct 11, 2019

Can we keep wp-scripts env to make everything easier for those who already started using it? This way @wordpress/env would be only an implementation detail for those who don't want to bother with all internal parts.

@youknowriad
Copy link
Contributor

youknowriad commented Oct 11, 2019

Can we keep wp-scripts env to make everything easier for those who already started using it? This way @wordpress/env would be only an implementation detail for those who don't want to bother with all internal parts.

Ideally, we do find a way to lazy-load @wordpress/scripts dependencies. It's not specific to the "env" command though.

Parcel does that a lot, I think we should take some inspiration from them and see how they do it.

@epiqueras epiqueras force-pushed the try/replacing-tests-env branch from 4a87158 to 0caa67e Compare October 11, 2019 20:38
@epiqueras
Copy link
Contributor Author

Can we keep wp-scripts env to make everything easier for those who already started using it? This way @wordpress/env would be only an implementation detail for those who don't want to bother with all internal parts.

I dropped the commit that removed it. @wordpress/env can't power wp-scripts env though, because the APIs and functionality differ too much. Maybe we should just add a deprecation message?

Parcel does that a lot, I think we should take some inspiration from them and see how they do it.

They have separate packages that are required dynamically by looking at the dependencies in package.json and testing each one with a RegExp, installing new packages if it needs them and they are not already installed.

So what we can do is break down each script into a separate package, then have the core of wp-scripts do this when ran:

  • Check if the script being called is already installed, install it if not.
  • Resolve its location in node_modules and require it.

@ntwb
Copy link
Member

ntwb commented Oct 12, 2019

I'd love to not see Downloading Chromium r686378 - 110.2 Mb [====================] 100%

Lazyloading just even this would make me quite happy ;)

@noisysocks
Copy link
Member

I dropped the commit that removed it. @wordpress/env can't power wp-scripts env though, because the APIs and functionality differ too much. Maybe we should just add a deprecation message?

I think a message is fine. This is how we replaced ./bin/setup-local-env.sh with npm run dev:

% ./bin/setup-local-env.sh
Hi there! It looks like you're trying to use the old local environment setup script. This script has been retired, running `npm run env install` will setup a local environment for you, instead.

Check out the documentation for more information: https://developer.wordpress.org/block-editor/contributors/develop/getting-started/

@noisysocks
Copy link
Member

I'm confused—why do we now need scripts/install-tests-wordpress-phpunit.sh?

@@ -195,13 +195,13 @@
"dev:packages": "node ./bin/packages/watch.js",
"docs:build": "node ./docs/tool/index.js && node ./bin/update-readmes.js",
"fixtures:clean": "rimraf \"packages/e2e-tests/fixtures/blocks/*.+(json|serialized.html)\"",
"fixtures:server-registered": "wp-scripts env docker-run php ./bin/get-server-blocks.php > test/integration/full-content/server-registered.json",
"fixtures:server-registered": "npm run env run tests-wordpress-phpunit './bin/get-server-blocks.php > test/integration/full-content/server-registered.json'",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should run this script with PHP, it doesn't use PHPUnit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but tests-wordpress-phpunit has php and we already use it.

@gziolo
Copy link
Member

gziolo commented Oct 17, 2019

I'd love to not see Downloading Chromium r686378 - 110.2 Mb [====================] 100%

Lazyloading just even this would make me quite happy ;)

See #15667. The issue is on the puppeteer side. The following code allows to skip it when it's executed as part of npm install:
https://github.com/GoogleChrome/puppeteer/blob/11ff374ca3e7d06a3be4278b85ccee73392354e7/install.js#L21-L32
In my opinion, this could be improved on the Puppeteer side.

@youknowriad
Copy link
Contributor

@gziolo I think we should do like parcel:

  • if you runwp-scripts test:e2e for the first time, the testing dependencies "puppeteer"... get added automatically to your package.json and dependencies installed before running the script
  • if your run wp-scripts env for the first time, the environment dependencies get added and installed...

This won't solve the issue for Gutenberg because we use all the dependencies but it's important for Plugin/Theme authors if they only use wp-scripts for bundling for instance.

@gziolo
Copy link
Member

gziolo commented Oct 17, 2019

Can we keep wp-scripts env to make everything easier for those who already started using it? This way @wordpress/env would be only an implementation detail for those who don't want to bother with all internal parts.

I dropped the commit that removed it. @wordpress/env can't power wp-scripts env though, because the APIs and functionality differ too much. Maybe we should just add a deprecation message?

Does it mean we can't match any of the commands so people don't have to install @wordpress/env separately? This is the whole idea behind @wordpress/scripts - you install one dependency and it provides you with everything you need to develop for WordPress.

We definitely should remove the old approach. We don't want to maintain two completely different environments.

@gziolo
Copy link
Member

gziolo commented Oct 17, 2019

@gziolo I think we should do like parcel:

  • if you runwp-scripts test:e2e for the first time, the testing dependencies "puppeteer"... get added automatically to your package.json and dependencies installed before running the script
  • if your run wp-scripts env for the first time, the environment dependencies get added and installed...

This won't solve the issue for Gutenberg because we use all the dependencies but it's important for Plugin/Theme authors if they only use wp-scripts for bundling for instance.

I love the idea, filed in #17987.

@epiqueras
Copy link
Contributor Author

@noisysocks

I'm confused—why do we now need scripts/install-tests-wordpress-phpunit.sh?

To bootstrap the Core PHP unit test framework unfortunately.

@gziolo

Does it mean we can't match any of the commands so people don't have to install @wordpress/env separately?

They don't line up 1-1. @wordpress/env just has start, stop, clean, and run. @wordpress/scripts env confusingly broke up things like start into multiple dependent steps you have to call in order.

I'm a bit torn now on whether we should use wordpress or wordpress-develop, see #17724 (comment).

It's a tradeoff between making this tool faster for plugin/theme developers and making it work for people who want to work on both codebases at once. Thoughts @youknowriad ?

@noisysocks
Copy link
Member

To bootstrap the Core PHP unit test framework unfortunately.

I see. Is it that previously we were cloning wordpress-develop which included the test framework?

It's a tradeoff between making this tool faster for plugin/theme developers and making it work for people who want to work on both codebases at once. Thoughts @youknowriad ?

Could it be a flag? Something like wp-env start --dev?

@epiqueras
Copy link
Contributor Author

I see. Is it that previously we were cloning wordpress-develop which included the test framework?

Yeah.

Could it be a flag? Something like wp-env start --dev?

Yes, it could be, and I guess we wouldn't need any more dependencies for it.

Should we go down that route? cc @youknowriad

@youknowriad
Copy link
Contributor

How big is the performance gap between both approaches?

wp-env start --dev

I'm fine with that. I'd personally prefer to have the least possible number of options, so being opinionated is better. If we can't manage to have both "fast" and "allow php unit testing" at the same time we can add that option.

@epiqueras
Copy link
Contributor Author

How big is the performance gap between both approaches?

Pretty significant. You have to install all of wordpress-develop's build dependencies and run the build on any changes.

I'm closing this and adding the new approach to the task list: #17724.

It'd be great if we could get the two pending PRs merged to avoid conflicts as we work on this. @youknowriad

@epiqueras epiqueras closed this Oct 21, 2019
@youknowriad youknowriad deleted the try/replacing-tests-env branch October 22, 2019 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants