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

Solve using DataViews in Jetpack products #39907

Open
simison opened this issue Oct 25, 2024 · 19 comments
Open

Solve using DataViews in Jetpack products #39907

simison opened this issue Oct 25, 2024 · 19 comments
Assignees
Labels
[Focus] Compatibility Ensuring our products play well with third-parties General [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it

Comments

@simison
Copy link
Member

simison commented Oct 25, 2024

We have multiple plugins using or will be using the DataViews: Social, Forms, Protect, Newsletters.

@manzoorwanijk noted:

I don't think DataViews can be used in Jetpack at the moment. We tried using DataViews in Social share status reporting, but Jetpack monorepo tooling wasn’t perfect enough and thus it didn’t allow us to use the component in our JS code. (Internal ref p1725458862906509/1725456184.868289-slack-C05Q5HSS013 )

So, we just used the same markup as that of the Dataviews component and imported only its styles. (internal ref p1725601980355649/1725456184.868289-slack-C05Q5HSS013 )

@anomiex noted:

The problem isn't the monorepo tooling, it's that Gutenberg's @wordpress/dependency-extraction-webpack-plugin doesn't extract the DataViews package, so it (along with its i18n messages and such) get included in each bundle.

From the summary of WordPress/gutenberg#56721 it seems they did that because they didn't consider the new package as being stable yet. Once it's stable they should have dependency-extraction-webpack-plugin start extracting it (file a bug with them if they forgot to do that). Then we wait for all supported versions of WordPress Core to include the wp-dataviews script handle or we add a polyfill (similar to #38428), then things in the monorepo should work.

If you really want to use it despite it still being considered experimental, there's a workaround for your webpack config in Slack (internal ref p1725461808826489/1725456184.868289-slack-C05Q5HSS013 ) that should allow it to be bundled successfully. Note any i18n strings from DataViews would need to be re-translated for Jetpack or whichever plugin, there's no good way to re-use core translations when the package has to be bundled. You'd also need to watch out for it being declared stable and starting to be extracted, e.g. make sure there's E2E test coverage of the feature.

Above is reference to adding below webpack config:

tools/webpack.config.extensions.js

            // Add textdomains (but no other optimizations) for @wordpress/dataviews.
            jetpackWebpackConfig.TranspileRule( {
                includeNodeModules: [
                    '@wordpress/dataviews/',
                ],
                babelOpts: {
                    configFile: false,
                    plugins: [
                        [
                            require.resolve( '@automattic/babel-plugin-replace-textdomain' ),
                            { textdomain: 'jetpack' },
                        ]
                    ],
                },
            } ),
@simison
Copy link
Member Author

simison commented Oct 25, 2024

Seems like we need to:

  • Check with Gutenberg team if DataViews package is stable enough to be externalized instead of bundling
  • Solve i18n, above one potential solution. Coordinate with i18n team to see if we can copy over the translations from Gutenberg without needing to re-translate the package twice.
  • See if we can avoid bundling multiple times; create a temporary Jetpack JS package with a export * from '@wordpress/dataviews' ?

@jeherve jeherve added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it General [Focus] Compatibility Ensuring our products play well with third-parties labels Oct 29, 2024
@pablinos
Copy link
Contributor

It's not quite the same problem, but there are issues with people using DataViews and ending up with a conflicting version of wordpress/components

There is a an approach to try and fix this WordPress/gutenberg#66825

@anomiex
Copy link
Contributor

anomiex commented Nov 20, 2024

That's helpful since we'd need a solution for the same problem too, in addition to the workaround for our i18n check.

If they get that merged, ping me and I'll look at setting things up in the monorepo to extract it and use that to make a polyfill.

Someone else would still need to deal with copying translations around if we want to worry about that part though. (If no one does, it'll just mean that any relevant strings get re-translated in every plugin).

@simison
Copy link
Member Author

simison commented Nov 20, 2024

Someone else would still need to deal with copying translations around if we want to worry about that part though. (If no one does, it'll just mean that any relevant strings get re-translated in every plugin).

cc @Automattic/i18n in case you could help here. Perhaps we can pull the strings from .org instead of running them through translators?

@dlind1
Copy link
Contributor

dlind1 commented Nov 20, 2024

cc @Automattic/i18n in case you could help here. Perhaps we can pull the strings from .org instead of running them through translators?

Is the problem extracting the strings, getting the translations or bundling them together with the plugin?
We've recently had some issues with DataViews not being translated in Calypso and have resolved it by extracting strings from node_modules/@wordpress/ and adding them to the Calypso project, so we should have translations on .com already and could reuse them if there is an agreed way on how to do that in Jetpack.

@anomiex
Copy link
Contributor

anomiex commented Nov 20, 2024

Jetpack and most other plugins in the Jetpack monorepo are translated via translate.wordpress.org, and get language packs downloaded from there by WordPress Core's normal mechanism. Exceptions include wpcomsh and jetpack-mu-wpcom-plugin; see pxLjZ-9e9-p2 for some discussion about those.

For the plugins on translate.wordpress.org I expect GlotPress will extract the strings from our bundles in the normal manner. The concern is that each plugin will have its own copy of the DataViews strings, making duplicate work for translators.

@dlind1
Copy link
Contributor

dlind1 commented Nov 21, 2024

The concern is that each plugin will have its own copy of the DataViews strings, making duplicate work for translators.

We have implemented syncing of Jetpack core translations into stand-alone plugins last year in pxLjZ-7QV-p2, so that should work for reusing the DataViews translations as well.

@manzoorwanijk
Copy link
Member

It simpy seems that we should probably forget about using Dataviews until it becomes stable.

@ilonagl
Copy link

ilonagl commented Nov 25, 2024

cc @nateweller, @dkmyta, @bhoop

@simison
Copy link
Member Author

simison commented Nov 25, 2024

@manzoorwanijk, does the above solve all the i18n issues, or are there still some outstanding ones?

@manzoorwanijk
Copy link
Member

I think if we solve the i18n issue, we should be good.

@manzoorwanijk
Copy link
Member

Warning folks ⚠

I just now tried to use dataviews in publicize-components in Jetpack and it crashed editor due to version mismatch as discussed in Slack - p1732619608113349-slack-C45SNKV4Z

@manzoorwanijk
Copy link
Member

That means, we should wait for WordPress/gutenberg#66825 to be merged and released before we can use dataviews.

@manzoorwanijk
Copy link
Member

Here is the PR

You can test it with WP 6.7.1 to see the crash after following the steps given there.

@pablinos
Copy link
Contributor

That means, we should wait for WordPress/gutenberg#66825

As we've been discussing in Slack. Let's see if we can gate whatever we develop to requiring Gutenberg and eventually a future release of WP. We run Gutenberg on WPCOM, can release things there and we can let Jetpack site owners know that they can opt in by running Gutenberg.

@manzoorwanijk
Copy link
Member

Since that PR from Riad got merged. I think the next step is to use its API and sort out the translations and we are good.

@anomiex
Copy link
Contributor

anomiex commented Dec 2, 2024

Looks like it just missed getting in the 4.9.0 version of the package, so first we'll need to wait for the next release.

@manzoorwanijk
Copy link
Member

We have another blocker for Dataviews

@anomiex
Copy link
Contributor

anomiex commented Dec 13, 2024

I took a look at making an automattic/jetpack-wordpress-dataviews-snapshot PHP package, and associated configuration in our webpack configs to make use of it to get a snapshot build of @wordpress/dataviews/wp. See #40612 for the in-progress PR.

  • The built package contains a build of @wordpress/dataviews/wp.
    • The minified file is 1.8M on disk.
  • The package, when activated, registers that script with a handle such as jetpack-wp-dataviews-snapshot-4.10.0.
    • The consumer will have to manually activate it early on. That way if one plugin uses dataviews 4.10.0 and another uses dataviews 4.11.0, both versioned handles will get registered and neither plugin will get an incompatible version of dataviews.
  • The monorepo Webpack config is set to error out if @wordpress/dataviews or @wordpress/dataviews/wp is used, with a pointer to instructions on how to use the automattic/jetpack-wordpress-dataviews-snapshot package.
  • A package or plugin using @wordpress/dataviews would have to update its webpack config as instructed to configure @wordpress/dependency-extraction-webpack-plugin to extract the dependency with the correct handle.

A downside to this is that our @automattic/jetpack-components package currently attempts to use @wordpress/dataviews in one component, and since the error check (bullet 3) happens before any tree-shaking then anything using @automattic/jetpack-components at all has a broken build now. If we go this route, that component should be moved to a different js-package (see also discussion here), or at least a non-default entry point, so code that doesn't use that one component doesn't hit the error.

OTOH, it's possible that this is overkill and including @wordpress/dataviews/wp directly into each plugin's bundle will turn out to be better. 🤷

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Compatibility Ensuring our products play well with third-parties General [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

No branches or pull requests

7 participants