DataViews: WP Specific build #66825
DataViews: WP Specific build #66825
Conversation
|
Size Change: 0 B Total Size: 1.82 MB ℹ️ View Unchanged
|
|
Flaky tests detected in e6108cc. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11724727915
|
|
I tested this using the WP Parse.ly plugin with this existing branch that implements a simple version of I did notice a couple other things:
|
|
I think I fixed the build in the last commit, so you won't have to the "react" error. |
| @@ -48,5 +48,8 @@ | |||
| }, | |||
| "publishConfig": { | |||
| "access": "public" | |||
| }, | |||
| "scripts": { | |||
| "build:wp": "esbuild src/index.ts --bundle --jsx=automatic --outdir=wp --external:react* --external:sprintf-js --external:hoist-non-react-statics --external:use-sync-external-store --external:deepmerge --external:fast-deep-equal --external:highlight-words-core --external:remove-accents --external:equivalent-key-map --external:rungen --external:moment* --external:clsx --external:is-plain-object --external:tslib --external:pascal-case --external:camel-case --external:change-case --external:lower-case --external:no-case --external:dot-case --external:memize --external:@tannin/* --external:tannin --format=esm --tree-shaking=true" | |||
There was a problem hiding this comment.
I spent some time on this and failed to generated the "external" list automatically. There's probably a way though. The idea is that I want to put any package dependency in the nested tree of dependencies as "external" unless it's @wordpress/*
There was a problem hiding this comment.
@luisherranz I know you spent some time exploring esbuild externals, maybe you could help here.
There was a problem hiding this comment.
Looking at esbuild's options, there's nothing about glob negation or a callback form of the option so I'm not sure how this could be done.
Webpack externals can use a function callback that allows fine control over what's external.
There was a problem hiding this comment.
What I wanted to do is a custom JS script that reads the package.json and the deep tree of dependencies and extract all dependencies that don't match @wordpress/*
There was a problem hiding this comment.
I'm not sure of the best way to handle that. DEWP does it to generate the asset files, it relies on webpack. I'm not sure how I'd go about walking the dependency tree, it's not simple.
I guess I'd rely on a bundler to do a first pass to collect all of the imported, then use that list to externalize the desired modules in a second pass.
Nice!! I tested it again, and it indeed worked correctly 💯 The build file also dropped in size a good chunk from Only two items that would still be of benefit I think:
Outside of that, I would be happy to move ahead with this. |
I'm personally surprised by the size as well. I wonder if "tree-shaking" is working properly. If feels the components package might be too big or not tree-shakable properly. It could be a good opportunity to understand why here. cc @ciampo @mirka
I'm not a fan of this idea personally, I'd rather use dependencies consistently and not try to "split" a package.
Not sure exactly how to validate that. I didn't do anything specific to indicate that the
yes, we need to do that. |
I think it is definitely doing some tree shaking, because not "all" of the components package gets bundled in, there is still quite a few items that are not in there. From a quick search items like
It could also be that I only saw the error first because the |
|
Would this PR also fix this: #65126? |
|
@ntsekouras if they switch to |
|
Any news on this? |
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
This is mostly ready, I think we need to test it extensively though so I would appreciate any help. Ideally as well, we should automate this list #66825 (comment) before merging as otherwise, any dependency change to components or something could break this again. I didn't find the right scripting to do it yet though. |
I tried to create a bundle that imports only the private APIs (of which 
Two things to see here:
The Also, the |
Yes, this is basically the goal here. I should be able to bundle a specific version of "components" in one part of the screen if I want without issues IMO. |
One of the problems that prevent doing that is that the Of course, we would create a new set of potential nightmares with versioning the platform libraries, dealing with breaking changes (in major semver updates), deprecations etc. Not sure if that's a good trade-off or not. I'm sure that @ciampo and @mirka already discussed this idea many times 🙂 |
That would solve the problem IMO, for instance slot-fill could also introduce new APIs that will be used by dataviews creating similar issues. I would love some testing to confirm whether we have actual issues or not. In my small testing, I didn't see any issues with having two bundles of components package (I mean aside the bundle size, but that could be acceptable). Also, the sub dependencies here are actually used in the parts that are imported by dataviews, so that's why they are not tree-shaken. |
CSS based on static class names (e.g. |
|
Given the complications involved when not externalizing the dataviews package into a WP bundle just like other packages, may be we should think about it a bit differently. SuggestionBundle The reason for my suggestion is that it's becoming very hard to actually use |
|
I understand where you're coming from and I agree that it's not really possible to use the dataviews package properly today in a Gutenberg plugin, and this is why I think solving this PR is high priority. I agree with @mirka that the styles conflicts is probably the only issue that we'll have but it's also a very small and acceptable one maybe? (depends on the breakage)
I would love to do that but unfortunately it is not possible at the moment to ship experimental/beta APIs in Core. That concept doesn't exist. |
|
We had a style conflict issue in Jetpack for Dataviews. We solved it in this PR |
|
@manzoorwanijk I'm curious how are you "bundling" DataViews today in Jetpack? |
We are not bundling it or using it as it's intended to. After the complications, we decided to use the same markup as that of So, for now we use only the styling of We have many use-cases of |
|
Thanks :) |
|
@manzoorwanijk Would anyone be able to test the package build from the current PR and see if it allows you to move forward. |
The problem for us in Jetpack is different than what this PR is trying to solve. Our problem is that |
Can you provide any basic instructions on how to test this? |
Related #63657
What?
This PR explores a temporary solution to conflicts and potential issues when using the "dataviews" package in WordPress while building your scripts using
wp-scripts.The issue is that you can endup with a version of DataViews that is more recent that the wordpress/components it could be relying on (because the components dependency is externalized) which causes JS errors.
The approach to fix this problem in the current PR is to ship within the npm package a "wp" specific build of the dataviews package.
So people using dataviews for their WordPress plugins would do
instead of just
And the trick here is that the "wp" build target in the npm package is already "pre bundled" and includes all the necessary wp-components dependency and its sub dependencies.
Downside:
Testing Instructions
This one is a bit hard to test (and I didn't test yet) but here's the idea:
npm run buildnpm link(to create a global symlink for the package)npm run link @wordpress/dataviews@wordpress/dataviews/wp