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. |
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? |
|
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? |
Can you provide any basic instructions on how to test this? |
One is to follow the testing instructions in the PR description (using npm link). The other way is to build the package, copy the folder |
|
I gave this PR a test using The only thing that doesn't seem to work well with the sub-path import is that |
|
I think it's clear that this PR is not a perfect solution (i18n and some style conflicts) but it seems the best that we have at the moment. So I propose that we fix the TS issues, try to simplify the build if possible and move forward with this approach. WDYT? |
I think that sounds like a good approach. Should we also add some experimental flag here to ensure that people don't rely on it as being something stable? like import something from '@wordpress/dataviews/wp__experimental';I know that's ugly but if there is a way to communicate it, that will be better and safer. |
|
@manzoorwanijk My experience with package subpaths exports is small and it's also the first time we use |
|
It turns out when you have an "exports" field, you need to define all the subpaths that can be consumed in there, so it's kind of impactful. I did it though, we just need to confirm that the types work properly. I was also able to avoid hardcoding the dependencies in the build script. So assuming the TS and exports fields are correct, this one should be ready. |
I tested it again and all seems good now. Thank you for fixing it. |
|
Thank you for testing @manzoorwanijk Seems we just need an approval here. cc @louwie17 @oandregal @jsnajdr :) |
youknowriad
added
[Type] Build Tooling
|
The types look good after fixing the It would be a big problem if there was any import from |
|
Thanks all. I'm guessing the package release is going to happen by the next Gutenberg release (forgot the dates and process exactly). Let me know how it goes and if there are any issues. Thank you. |
|
@youknowriad We already have a 19.8 RC 1. It seems it would be helpful to have this PR in the final release next week? If yes, I would label it (that would also fix the issue wptrainingteam/devblog-dataviews-plugin#2 (comment) sooner) |
youknowriad
added
the
Backport to Gutenberg RC
|
Good call, I added the label. |
fixes #65126
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