Build: Use globalThis over process.env and enable TS lib checking #61486
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sirreal
force-pushed
the
fix/skip-lib-check-breakage
branch
2 times, most recently
from
ccb658c to
40c0fd7
Compare
sirreal
added a commit
that referenced
this pull request
May 8, 2024
Squashed commit of the following: commit 40c0fd7 Author: Jon Surrell <sirreal@users.noreply.github.com> Date: Wed May 8 18:55:56 2024 +0200 Patch react-autosize-textarea for updated types commit 69cf754 Author: Jon Surrell <sirreal@users.noreply.github.com> Date: Wed May 8 18:48:37 2024 +0200 upgrade framer-motion commit b383044 Author: Jon Surrell <sirreal@users.noreply.github.com> Date: Wed May 8 18:34:44 2024 +0200 Upgrade @use-gesture/react commit fdf93ae Author: Jon Surrell <sirreal@users.noreply.github.com> Date: Wed May 8 18:32:11 2024 +0200 Go bonkers on the global vars commit cc43b8c Author: Jon Surrell <sirreal@users.noreply.github.com> Date: Wed May 8 16:46:39 2024 +0200 Fix the process problem commit 548145e Author: Jon Surrell <sirreal@users.noreply.github.com> Date: Wed May 8 13:33:23 2024 +0200 No more skipLibCheck commit 1eb7644 Author: Miguel Fonseca <150562+mcsf@users.noreply.github.com> Date: Wed May 8 16:33:10 2024 +0100 build: Suggest workaround if `tsc --build` fails (#61501) Following the merge of #60796, developers may face build issues that require package types to be rebuilt. The problem is that `tsc --build` fails somewhat silently -- or rather, there is some output, but it's not clear in the console which stage of the `build:package-types` command failed, and hence what the workaround should be. This commit alleviates the issue by logging a helpful message in the console if `tsc --build` fails: tsc failed. Try cleaning up first: `npm run clean:package-types` commit 14ecb1d Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed May 8 09:49:20 2024 -0400 Bump actions/checkout from 4.1.4 to 4.1.5 in the github-actions group (#61449) Bumps the github-actions group with 1 update: [actions/checkout](https://github.com/actions/checkout). Updates `actions/checkout` from 4.1.4 to 4.1.5 - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@0ad4b8f...44c2b7a) --- updated-dependencies: - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-patch dependency-group: github-actions ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: desrosj <desrosj@git.wordpress.org> commit e899475 Author: Marin Atanasov <8436925+tyxla@users.noreply.github.com> Date: Wed May 8 16:48:50 2024 +0300 RadioControl: Fix shrinking radio controls (#61476) Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org> commit 839c17d Author: Riad Benguella <benguella@gmail.com> Date: Wed May 8 14:39:34 2024 +0100 Editor: Unify Header component. (#61273) Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: jeryj <jeryj@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> commit 2c2f899 Author: Ella <4710635+ellatrix@users.noreply.github.com> Date: Wed May 8 20:30:44 2024 +0900 Revert "useBlockSync: remove isControlled effect" (#61480) Co-authored-by: ellatrix <ellatrix@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
|
Size Change: 0 B Total Size: 1.74 MB ℹ️ View Unchanged
|
youknowriad
reviewed
May 9, 2024
gziolo
reviewed
May 10, 2024
There was a problem hiding this comment.
I like the direction. Thank you for taking care of the additional refactoring for the special constants we use to guard some features like IS_GUTENBERG_PLUGIN, IS_WORDPRESS_CORE and SCRIPT_DEBUG.
One file still needs to get updated:
https://github.com/WordPress/gutenberg/blob/trunk/test/unit/config/gutenberg-env.js
If I follow it correctly, it sets the default values for unit tests, now it should be set on globalThis.*.
| @@ -64,13 +64,15 @@ const plugins = [ | |||
| process.env.WP_BUNDLE_ANALYZER && new BundleAnalyzerPlugin(), | |||
| new DefinePlugin( { | |||
| // Inject the `IS_GUTENBERG_PLUGIN` global, used for feature flagging. | |||
| 'process.env.IS_GUTENBERG_PLUGIN': | |||
| process.env.npm_package_config_IS_GUTENBERG_PLUGIN, | |||
| 'globalThis.IS_GUTENBERG_PLUGIN': JSON.stringify( | |||
There was a problem hiding this comment.
I like the direction. We will have to remember to update the WordPress Core so tree-shaking continues to work.
There was a problem hiding this comment.
Adding the "php backport" label. Although it's not really PHP :)
There was a problem hiding this comment.
What is to be backported? Is this file shared with core?
There was a problem hiding this comment.
The webpack config change.
There was a problem hiding this comment.
It would be a very small and simple (but important!) change. I'll open a PR for that soon so it's ready.
There was a problem hiding this comment.
This was referenced May 10, 2024
Merged
Merged
sirreal
force-pushed
the
fix/skip-lib-check-breakage
branch
from
59daff2 to
1400f63
Compare
Thanks! I agree, I think this will be easier to work with 👍
Good spot! Maybe we should also switch the global |
|
There are several eslint rules around how to use gutenberg/packages/eslint-plugin/rules/is-gutenberg-plugin.js Lines 80 to 84 in 5915f9a gutenberg/packages/eslint-plugin/rules/gutenberg-phase.js Lines 162 to 172 in 5915f9a (Maybe GUTENBERG_PHASE needs to be removed or updated to simply ban the variable?) |
|
Docs need an update: |
sirreal
force-pushed
the
fix/skip-lib-check-breakage
branch
6 times, most recently
from
e1b077e to
b33d096
Compare
sirreal
force-pushed
the
fix/skip-lib-check-breakage
branch
from
b33d096 to
3980ff3
Compare
|
This should have breaking change changelog for all the updated packages- maybe for all packages- mentioning the change to |
sirreal
force-pushed
the
fix/skip-lib-check-breakage
branch
from
d005d5b to
4110165
Compare
sirreal
commented
May 14, 2024
| ### Breaking Changes | ||
|
|
||
| - `@wordpress/is-gutenberg-plugin` rule has been replaced by `@wordpress/wp-global-usage` ([#61486](https://github.com/WordPress/gutenberg/pull/61486)). | ||
| - `@wordpress/wp-process-env` rule has been added and included in the recommended configurations ([#61486](https://github.com/WordPress/gutenberg/pull/61486)). |
There was a problem hiding this comment.
@wordpress/gutenberg-phase rule removed. That doesn't exist and hasn't been used for some time.
There was a problem hiding this comment.
Yes, it stopped being a thing after WordPress Phase 2 was concluded - sometime between WP 5.9 and 6.3 when site editor was going out of beta.
sirreal
force-pushed
the
fix/skip-lib-check-breakage
branch
from
86561d4 to
926ab27
Compare
sirreal
added
Needs Dev Note
|
I added and removed the dev note label. Dev notes are largely for WordPress developers and these changes should not affect them at all. They'll be using the included Scripts from WordPress that already have the optimizations applied. This change already includes breaking change changelogs for folks consuming the packages on npm, that's probably the best place for it. |
|
@sirreal, I see the following error in the editor after this got merged. Is there an additional step to get it fixed? Screenshot
|
|
@Mamaduka I think that's probably a stale process or something. Try running |
|
Thanks, @sirreal! The |
pento
pushed a commit
to WordPress/wordpress-develop
that referenced
this pull request
May 24, 2024
This updates the build to account for related changes in WordPress packages. More details in WordPress/gutenberg#61486. Props jonsurrell, youknowriad, swissspidy, gziolo. Fixes #61262. git-svn-id: https://develop.svn.wordpress.org/trunk@58193 602fd350-edb4-49c9-b593-d223f7449a82
github-actions bot
pushed a commit
to platformsh/wordpress-performance
that referenced
this pull request
May 24, 2024
This updates the build to account for related changes in WordPress packages. More details in WordPress/gutenberg#61486. Props jonsurrell, youknowriad, swissspidy, gziolo. Fixes #61262. Built from https://develop.svn.wordpress.org/trunk@58193 git-svn-id: https://core.svn.wordpress.org/trunk@57656 1a063a9b-81f0-0310-95a4-ce76da25c4cd
markjaquith
pushed a commit
to WordPress/WordPress
that referenced
this pull request
May 24, 2024
This updates the build to account for related changes in WordPress packages. More details in WordPress/gutenberg#61486. Props jonsurrell, youknowriad, swissspidy, gziolo. Fixes #61262. Built from https://develop.svn.wordpress.org/trunk@58193 git-svn-id: http://core.svn.wordpress.org/trunk@57656 1a063a9b-81f0-0310-95a4-ce76da25c4cd
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Jun 4, 2024
…rdPress#61486) # Change `process.env.NAME` to `globalThis.NAME`. This includes changing `SCRIPT_DEBUG` global to use `globalThis.SCRIPT_DEBUG`. There are a few problems with `process.env`: - It only exists in Node.js. It does not exist in the browser. This means that additional checks must be added in application code to make sure that it doesn't error. - `process.env` in Node.js is unusual. It's a key-value structure where _the values are always strings_. See https://nodejs.org/docs/latest-v20.x/api/process.html#processenv Gutenberg used non-string values which was wrong in environments where `process.env` exists, but also did compile-time replacement in some environments with impossible values. This makes everything confusing. - The inconsistent `process.env` values caused problems in the type system. They conflicted with the Node.js types that correctly assert that process.env values must be `strings`. Using `globalThis` has the benefit of being safer in third party code and valid in the type system. # Remove all skipLibCheck from tsconfig The `skipLibCheck` configuration is no longer necessary and helps to produce better type declarations for Gutenberg's published libraries. --------- Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>
patil-vipul
pushed a commit
to patil-vipul/gutenberg
that referenced
this pull request
Jun 17, 2024
…rdPress#61486) This includes changing `SCRIPT_DEBUG` global to use `globalThis.SCRIPT_DEBUG`. There are a few problems with `process.env`: - It only exists in Node.js. It does not exist in the browser. This means that additional checks must be added in application code to make sure that it doesn't error. - `process.env` in Node.js is unusual. It's a key-value structure where _the values are always strings_. See https://nodejs.org/docs/latest-v20.x/api/process.html#processenv Gutenberg used non-string values which was wrong in environments where `process.env` exists, but also did compile-time replacement in some environments with impossible values. This makes everything confusing. - The inconsistent `process.env` values caused problems in the type system. They conflicted with the Node.js types that correctly assert that process.env values must be `strings`. Using `globalThis` has the benefit of being safer in third party code and valid in the type system. The `skipLibCheck` configuration is no longer necessary and helps to produce better type declarations for Gutenberg's published libraries. --------- Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>
sirreal
added a commit
to sirreal/wordpress-develop
that referenced
this pull request
Jun 25, 2024
WordPress/gutenberg#61486 changed the variables compiled into Gutenberg packages via the webpack DefinePlugin. https://core.trac.wordpress.org/changeset/58193 added the new DefinePlugin configuration, but did not remove the legacy configuration in order to be compatible with new and old Gutenberg versions before the packages were updated. Now that packages have been update, the legacy configuration should be removed. Closes https://core.trac.wordpress.org/ticket/61262. Follow-up to https://core.trac.wordpress.org/changeset/58193.
This was referenced Jun 25, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
There have been some problems with types in some packages. This resulted in build errors and in producing types that were not valid. This was observed in several other PRs recently.
This PR does two things:
skipLibCheckfrom tsconfig.process.env.NAMEusage in the application toglobalThis.NAME. This has the benefit of being safer in third party code and valid in the type system.There are a few problems with using
process.env, although it's common to use some names for historic reasons - primarilyprocess.env.NODE_ENV.process.envin general is error prone.process.envin Node.js is unusual. It's a key-value structure where the values are always strings. Gutenberg used non-string values which was wrong in environments where process.env existed, but also did compile-time replacement sometimes. This makes everything confusing.strings.Instead of using the problematic
process.env"object" for these values, we change to useglobalThis. This is the global object that works natively in both browsers and Node.js. It is an object and can be used as such, so assigning arbitrary properties is straightforward.One big benefit is that
globalThisis much easier to use,globalThis.nonExistentPropertyisundefinedinstead of aReferenceErrorlike when trying to accessprocess.env.nonExistentPropertyin the browser.This eliminates the type issues around using
process.envand is generally more coherent. The build tooling and code using WordPress and Gutenberg variables aroundprocess.envhas been updated in this PR. Documentation has also been updated where I found it.Some eslint rules have also been added, changed, or removed:
@wordpress/wp-global-usagereplaces@wordpress/is-gutenberg-plugin. It applies the same logic to all the globals that Gutenberg uses - namely that they should be used in simple conditional tests to help with dead code elimination in compiled packages.@wordpress/wp-process-envis a new rule that makes the usage ofprocess.envWordPress/Gutenberg variables a violation. It includes a fixer, so e.b.process.env.SCRIPT_DEBUGbecomesglobalThis.SCRIPT_DEBUG.@wordpress/gutenberg-phaserule is removed - this variable has not been used for some time.Related changes:
Why?
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast