Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 29 additions & 2 deletions .github/workflows/check-package-changelogs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,22 @@ on:
branches:
- trunk
paths:
- 'packages/admin-ui/**'
- 'packages/components/**'
- 'packages/dataviews/**'
- 'packages/ui/**'
- 'packages/fields/**'
- 'packages/grid/**'
Comment thread
simison marked this conversation as resolved.
- 'packages/icons/**'
- 'packages/interface/**'
- 'packages/private-apis/**'
- 'packages/style-runtime/**'
- 'packages/theme/**'
- 'packages/ui/**'
- 'packages/undo-manager/**'
- 'packages/views/**'
- 'packages/widget-dashboard/**'
- 'packages/widget-primitives/**'
- 'packages/wp-build/**'
- '!packages/*/src/**/stories/**'
- '!packages/*/src/**/test/**'

Expand All @@ -27,10 +39,25 @@ jobs:
fail-fast: false
matrix:
package:
- components
# Bundled packages
# Keep in sync with dependency-extraction-webpack-plugin

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The list:

const BUNDLED_PACKAGES = [
'@wordpress/admin-ui',
'@wordpress/dataviews',
'@wordpress/dataviews/wp',
'@wordpress/fields',
'@wordpress/grid',
'@wordpress/icons',
'@wordpress/interface',
'@wordpress/style-runtime',
'@wordpress/ui',
'@wordpress/undo-manager',
'@wordpress/views',
];

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I sense the answer is a reluctant "yes", but do we still maintain @wordpress/dependency-extraction-webpack-plugin? At the very least, I think the way I'd think we should document this expectation is around our current thinking with wpScript / wpScriptModule and how that's handled through @wordpress/build. And if we intend to maintain @wordpress/dependency-extraction-webpack-plugin, it should probably be updated to use those same indicators instead of including a hard-coded list. A related thought I had is that if we wanted to keep this in sync, we could do some sort of CI inception to force-keep it in sync 😂 (something like #76263)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, I don't think we're moving away from @wordpress/scripts either super soon (or at least we need to maintain it for now).

That said, I think we could move to read package.json files in dependency-extraction-webpack-plugin too instead of the hardcoded list. Actually, it would make it so that the version of dependency-extraction-webpack-plugin doesn't depend on matching specific versions of those bundled packges; the package itself tells if it should be bundled or not.

I also want to be transparent to consumer what gets bundled and what externalized, because it really affects a lot how careful they need to be when updating dependency versions. Doing that in https://github.com/WordPress/gutenberg/pull/79634/changes#diff-ed1ee7b58dba4945d02b57bbde06ff95123fc15955ae2d760f1707b00b22fb56 but later iterations could change how we expose the info.

For now I just want to make sure changelogs are updated within the setup we have today, and we can improve the setup outside this PR.

- admin-ui
- dataviews
- fields
- grid
- icons
- interface
- style-runtime
- ui
- undo-manager

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

undo-manager isn't a bundled package, is it? It has wpScript: true:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess per your other comment it's consistent with what's in the Webpack plugin, but now I'm confused why it's there 🤔

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It turns out to be a fun little rabbit-hole.

The package was originally intended to be "bundled", as mentioned in @youknowriad 's comment #54292 (comment).

As part of shifting to npm workspaces in #65681 / #66272, wpScript was added to packages that are shipped as WordPress globals. At the time, per @sirreal 's comment at #65681 (comment), I believe this was largely meant for license check purposes only. wpScript was mistakenly added to @wordpress/undo-manager.

The @wordpress/build package added in #72032 used wpScript to detect whether to create the WordPress global. Some packages were revised as part of this work to remove wpScript (icons and interface in #72319), but undo-manager kept it.

Thus, we now have a state where @wordpress/undo-manager is shipped as a WordPress global window.wp.undoManager in WordPress 7.0 (confirmed locally), despite originally intended to be a bundled package.

In any case, not really a blocker for this pull request in particular, but we'll need to figure out what we want to do with this package.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I did some archaeology, and the entry is simply stale: undo-manager was added to BUNDLED_PACKAGES in #54292 (September 2023), when the package had just been extracted from core-data and wasn't yet a core script.

It later became one ("wpScript": true today), but it was never removed from the hard-coded list. It's the only package of the ten in that list with wpScript: true; the other nine have it absent or false.

For this PR, the grouping is cosmetic; the check behaves the same regardless of where the package sits.

The real fix would be removing undo-manager from BUNDLED_PACKAGES in dependency-extraction-webpack-plugin, but that changes what consumers bundle vs externalize, so it deserves its own PR.

Which also reinforces the point you both made: deriving this from package.json flags would have prevented the drift in the first place.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

- views
# Other packages
- theme
- components
- private-apis
- widget-dashboard
- widget-primitives
- wp-build
steps:
- name: 'Get PR commit count'
env:
Expand Down
Loading