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

Babel macro: Add new Babel macro which handles block.json file transformation #16088

Closed
wants to merge 1 commit into from

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Jun 11, 2019

Description

Input

{
	"name": "my-plugin/notice",
	"title": "Notice",
	"category": "common",
	"description": "Shows warning, error or success notices  ...",
	"keywords": [ "alert", "message" ],
	"textDomain": "default",
	"attributes": {},
	"styleVariations": [
		{ "name": "default", "label": "Default", "isDefault": true },
		{ "name": "other", "label": "Other" }
	]
}
import getBlockData from '@wordpress/blocks/macro';
const metadata = getBlockData( './fixtures/block-i18n.json' );

Output

\\"use strict\\";

var _i18n = require(\\"@wordpress/i18n\\");

const metadata = {
  name: \\"my-plugin/notice\\",
  title: (0, _i18n._x)(\\"Notice\\", \\"block title\\", \\"my-plugin\\"),
  category: \\"common\\",
  description: (0, _i18n._x)(\\"Shows warning, error or success notices  ...\\", \\"block description\\", \\"my-plugin\\"),
  keywords: [(0, _i18n._x)(\\"alert\\", \\"block keywords\\", \\"my-plugin\\"), (0, _i18n._x)(\\"message\\", \\"block keywords\\", \\"my-plugin\\")],
  attributes: {},
  styles: [{
    name: \\"default\\",
    label: (0, _i18n._x)(\\"Default\\", \\"block styles\\", \\"my-plugin\\"),
    isDefault: true
  }, {
    name: \\"other\\",
    label: (0, _i18n._x)(\\"Other\\", \\"block styles\\", \\"my-plugin\\")
  }]
};

TODO

  • Use Babel to inline block.json file, accept only whitelist properties and wrap those translatable with i18n calls
  • Make it work with at least one existing block
  • Decide whether we should Babel macro or regular plugin
  • Add cache invalidation for webpack and Jest when using a macro
  • Add documentation

Related resources:

Examples:

import preval from 'preval.macro'
import idx from 'idx.macro'

// preval macro is evaluated first, then idx

@gziolo gziolo force-pushed the add/babel-block-macro branch from 6de5d19 to dc4afea Compare June 11, 2019 11:02
@gziolo gziolo added [Feature] Block Directory Related to the Block Directory, a repository of block plugins [Feature] Block API API that allows to express the block paradigm. [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible [Status] In Progress Tracking issues with work in progress labels Jun 11, 2019
@gziolo gziolo self-assigned this Jun 11, 2019
@talldan
Copy link
Contributor

talldan commented Jun 12, 2019

Hi @gziolo - something I noticed recently is that changes to the block.json files aren't built during watch. I haven't looked into how the build for json files works. Is that something that this PR will fix or should it be tackled separately?

@gziolo
Copy link
Member Author

gziolo commented Jun 12, 2019

something I noticed recently is that changes to the block.json files aren't built during watch. I haven't looked into how the build for json files works. Is that something that this PR will fix or should it be tackled separately?

I noticed it as well. This PR doesn’t address it in the current shape but there are some existing solutions in creste-react-app showing how to enforce re-building files which use macros. I think the same issue applies to Jest. We can look into it separately.

@talldan
Copy link
Contributor

talldan commented Jun 12, 2019

👍 I've created an issue - #16104

@gziolo gziolo force-pushed the add/babel-block-macro branch from dc4afea to 464b0bd Compare September 9, 2019 14:40
@gziolo gziolo marked this pull request as ready for review September 11, 2019 10:47
@gziolo gziolo force-pushed the add/babel-block-macro branch 2 times, most recently from 498d730 to 2054fb5 Compare September 15, 2019 21:29
@gziolo gziolo requested a review from ellatrix as a code owner September 15, 2019 21:29
@gziolo
Copy link
Member Author

gziolo commented Sep 15, 2019

I updated PR to allow the following usage in the block's file:

import getBlockData from '@wordpress/blocks/macro';
const metadata = getBlockData( './fixtures/block-i18n.json' );

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

As to whether we should go down the route of this being a macro: From some of the readings, it doesn't seem to me that the plugin should really need to be overly opinionated one way or the other. Would it not be too much trouble to just make a normal babel transform plugin, whose logic is reused by a bundled macro, then the user could choose which to use based on their own preferences?

I see some advantages in macros, certainly (as highlighted in the Babel blog post). It's a bit funny to me one of the promoted advantages is the lack of configuration necessary, when in-fact you do need to configure at least the babel-plugin-macros plugin. Maybe in the future this could become a built-in feature of Babel. The "Babel cache" caveat seems concerning, though the messaging on it is mixed. If it can have the potential to impact consumers in how they use the macro, that seems really non-ideal. It also doesn't seem to have had much visible movement for solutions since evenchange4/graphql.macro#6 was opened almost two years ago.

It really got me pushing to consider what would be necessary to avoid any of these transforms at all. Of course, we could force someone to wrap the translatable strings themselves when registering the block. There's a lot less magic here, which can be a good thing, but it's also very inconvenient.

I wonder also about whether we can do it at the framework based on the namespace provided when a block is registered.

i.e. Would something like this be feasible?

import settings from './block.json';

registerBlockType( 'my-plugin/map', settings )
function registerBlockType( name, settings ) {
    const [ namespace ] = name.split( '/' );

    settings = {
        ...settings,
        title: _x( settings.title, namespace ),
        // ...
    };
}

const { mapKeys, pick } = require( 'lodash' );
const { dirname, join, relative } = require( 'path' );

const i18nVariable = '_x';
Copy link
Member

Choose a reason for hiding this comment

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

I don't imagine there's much value in making this configurable, since _x specifically assumes to be used with an additional argument for the context, which in this implementation is hard-coded and not-configurable.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree I don't remember why I introduced it :)

throw new Error( `Invalid file name provided: ${ filename }.` );
}

const metadataRaw = readFileSync( filename, 'utf8' );
Copy link
Member

Choose a reason for hiding this comment

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

Is there any way to make this non-blocking?

Copy link
Member Author

Choose a reason for hiding this comment

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

I looked at 2 or 3 plugins that read JSON files and they use a blocking approach. I'm happy to use the async version but I would need some guidelines or any working example. Do you know anything that would be useful?

} );
}
} else {
throw new Error(
Copy link
Member

Choose a reason for hiding this comment

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

We could do some early returns to avoid the nesting

if ( referencePath.parentPath.type !== 'CallExpression' ) {
	throw new Error();
}

// ...

if ( ! textDomain ) {
	return;
}

// ...

Copy link
Member Author

Choose a reason for hiding this comment

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

I followed one of the most popular macros when writing the proof of the concept. You are absolutely right, we should refactor 👍

@gziolo gziolo force-pushed the add/babel-block-macro branch from 2054fb5 to 78592e1 Compare December 6, 2019 14:11
@gziolo
Copy link
Member Author

gziolo commented Dec 6, 2019

As to whether we should go down the route of this being a macro: From some of the readings, it doesn't seem to me that the plugin should really need to be overly opinionated one way or the other. Would it not be too much trouble to just make a normal babel transform plugin, whose logic is reused by a bundled macro, then the user could choose which to use based on their own preferences?

I'm happy to provide both. It feels like it shouldn't be too much work to add support for the regular plugin and perform the transform when block.json is detected.

I see some advantages in macros, certainly (as highlighted in the Babel blog post). It's a bit funny to me one of the promoted advantages is the lack of configuration necessary, when in-fact you do need to configure at least the babel-plugin-macros plugin. Maybe in the future this could become a built-in feature of Babel. The "Babel cache" caveat seems concerning, though the messaging on it is mixed. If it can have the potential to impact consumers in how they use the macro, that seems really non-ideal. It also doesn't seem to have had much visible movement for solutions since evenchange4/graphql.macro#6 was opened almost two years ago.

The truth is that the issue with cache exists for both macros and regular plugins in case when you introduce a dependency on non-JS asset. It's also something that isn't resolved in Babel core until today.

It really got me pushing to consider what would be necessary to avoid any of these transforms at all. Of course, we could force someone to wrap the translatable strings themselves when registering the block. There's a lot less magic here, which can be a good thing, but it's also very inconvenient.

In fact, this is what PHP does for the plugin registration. So maybe we could do the same.

I wonder also about whether we can do it at the framework based on the namespace provided when a block is registered.

i.e. Would something like this be feasible?

import settings from './block.json';

registerBlockType( 'my-plugin/map', settings )
function registerBlockType( name, settings ) {
    const [ namespace ] = name.split( '/' );

    settings = {
        ...settings,
        title: _x( settings.title, namespace ),
        // ...
    };
}

It makes me wonder how exactly the translations system works. I assumed that you need to ship code which has to wrap all translations with i18n function so they could be detected. That's why I opted for the Babel approach where it explicitly includes the string wrapped with i18n functions so it can be detected. @swissspidy - can you provide more details? Which approach would you pick? Maybe it doesn't need to be as magical as I wanted it to be :)

@gziolo gziolo requested a review from swissspidy December 6, 2019 20:55
@gziolo gziolo added the Internationalization (i18n) Issues or PRs related to internationalization efforts label Dec 6, 2019
@gziolo
Copy link
Member Author

gziolo commented Dec 9, 2019

It makes me wonder how exactly the translations system works. I assumed that you need to ship code which has to wrap all translations with i18n function so they could be detected. That's why I opted for the Babel approach where it explicitly includes the string wrapped with i18n functions so it can be detected.

@aduth, I see you filed #19006 where you discovered that translator comments get removed when JS code is bundled in the plugin code. It only assures me that this is why this whole Babel story has been started. Those translations are read from JS code. The same issue will exist for PHP code when we opt for dynamically applied translations. It means we have two options:

  • parse all block.json files when extracting strings to translate for the plugin
  • go with the Babel transformation which will handle it with the current system

@swissspidy
Copy link
Member

It makes me wonder how exactly the translations system works. I assumed that you need to ship code which has to wrap all translations with i18n function so they could be detected. That's why I opted for the Babel approach where it explicitly includes the string wrapped with i18n functions so it can be detected.

@gziolo The string extraction system (WP-CLI) would ideally extract the strings directly from block.json so translators can translate them. That of course means the block.json files need to be shipped with the plugin / core. However, you still need to run any strings through the __()/_x() function somewhere so the translations are actually applied. I suppose that's what this macro does?

@gziolo
Copy link
Member Author

gziolo commented Dec 11, 2019

However, you still need to run any strings through the __()/_x() function somewhere so the translations are actually applied. I suppose that's what this macro does?

In fact, this is what @aduth proposed as an alternative solution. If we want to start using block.json with PHP we will have to ship them with the plugin so we meet the criteria. The challenge would be only how WP-CLI locates all block.json files. On the Gutenberg side, we would also need to update the build step to expose them.

Do you know who could help with implementation efforts on WP-CLI side? I'll open an issue there as soon as I confirm this with @aduth and @youknowriad.

@swissspidy
Copy link
Member

I'll open an issue there as soon as I confirm this

Feel free to open an issue at https://github.com/wp-cli/i18n-command/

I can potentially help with implementation as I built large parts of that tool and am familiar with it. Then, I suppose @ocean90 could help update it on dotorg.

The challenge would be only how WP-CLI locates all block.json files

Just like WP-CLI scans all JS and PHP files for translation functions, it would then also scan all block.json files that are deemed valid. Don't see a huge difficulty there.

@gziolo
Copy link
Member Author

gziolo commented Jan 3, 2020

Let's close this PR based on the feedback received. Thank you for all comments that helped to figure out the next steps 🙇

@gziolo gziolo closed this Jan 3, 2020
@gziolo gziolo deleted the add/babel-block-macro branch January 3, 2020 13:28
@gziolo gziolo removed the [Status] In Progress Tracking issues with work in progress label Jan 3, 2020
@gziolo
Copy link
Member Author

gziolo commented Jan 3, 2020

@swissspidy - it turned out you already filed an issue a few months back, I left a comment there: wp-cli/i18n-command#163 (comment).

@gziolo
Copy link
Member Author

gziolo commented Jan 3, 2020

I also updated the development roadmap in #16209 to reflect comments from @swissspidy and @aduth:

Screen Shot 2020-01-03 at 14 49 45

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. [Feature] Block Directory Related to the Block Directory, a repository of block plugins Internationalization (i18n) Issues or PRs related to internationalization efforts [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants