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

Packages: Create new `spec-parser` package #7664

Merged
merged 3 commits into from Jul 19, 2018

Conversation

@gziolo
Member

gziolo commented Jul 2, 2018

Description

This PR follows the setup for Simplenote app: https://github.com/Automattic/simplenote-grammar-pegjs where grammar is published to npm together with a generated JS source file.

This change allows to use Gutenberg grammar without Webpack config and simplifies setup we have. It makes it also much easier to use gramma parser outside of Gutenberg.

Implications

Downside of this approach is that we won't be able to iterate as easily as now on the parser's content. What we had so far allows to use Webpack to dynamically update code using pegjs-loader. Question is, if we really need it becaue pegjs file was updated exactly 3 times this year by @mcsf:
https://github.com/WordPress/gutenberg/commits/master/blocks/api/post.pegjs

How has this been tested?

npm test stil works
npm run dev still works
npm run build still works

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@gziolo gziolo added the Packages label Jul 2, 2018

@gziolo gziolo added this to the 3.2 milestone Jul 2, 2018

@gziolo gziolo self-assigned this Jul 2, 2018

@gziolo gziolo requested review from mcsf, hypest, aduth, WordPress/gutenberg-core and mtias Jul 2, 2018

@mcsf

So far I'm thinking this is a worthwhile change. 👍

Show outdated Hide outdated .eslintrc.js
Show outdated Hide outdated bin/create-php-parser.js
Show outdated Hide outdated packages/grammar/README.md
@hypest

This comment has been minimized.

Show comment
Hide comment
@hypest

hypest Jul 2, 2018

Contributor

Good to see this @gziolo ! Will test it out with the RN app and report back.

Contributor

hypest commented Jul 2, 2018

Good to see this @gziolo ! Will test it out with the RN app and report back.

@gziolo gziolo referenced this pull request Jul 4, 2018

Merged

Expose the grammar parser to the mobile app #7691

4 of 4 tasks complete
@hypest

Gave the PR a try on the native mobile app @gziolo .

Had to install pegjs globally first and also manually tweak the element package entrypoint (removing the .js extension as we discuss elsewhere).

The results are mixed I'd say:

  • The parser is generated and loaded just fine
  • The (RN) parser tests fail for the more block though. Here's the result:
  console.error gutenberg/blocks/api/validation.js:147
    Block validation failed for `core/more` (0).

    Expected:

    <!--more-->

    Actual:

    <!--more-->

That's an error I remember seeing in the past when I was first trying to port the parser to mobile. If I recall correctly, I resolved it by finding a different way to get a hold to the generated parser.

Here's a branch on the RN repo in case you want to have a look.

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Jul 4, 2018

Member

That's an error I remember seeing in the past when I was first trying to port the parser to mobile. If I recall correctly, I resolved it by finding a different way to get a hold to the generated parser.

We need to replicate what you did in the past because we need to be able to update parser without doing manual steps. In addition, it should work for all platforms with one JS codebase if possible.

Member

gziolo commented Jul 4, 2018

That's an error I remember seeing in the past when I was first trying to port the parser to mobile. If I recall correctly, I resolved it by finding a different way to get a hold to the generated parser.

We need to replicate what you did in the past because we need to be able to update parser without doing manual steps. In addition, it should work for all platforms with one JS codebase if possible.

@hypest

This comment has been minimized.

Show comment
Hide comment
@hypest

hypest Jul 4, 2018

Contributor

We need to replicate what you did in the past

OK, I think I found what I did in the past: I tapped into the test/unit/pegjs-transform.js code and added a "save to file" snippet. Here's what I think I used:

const pegjs = require( 'pegjs' );

module.exports = {
	process( src ) {
		// Description of PEG.js options: https://github.com/pegjs/pegjs#javascript-api
		const pegOptions = {
			output: 'source',
			cache: false,
			optimize: 'speed',
			trace: false,
		};
		const methodName = ( typeof pegjs.generate === 'function' ) ? 'generate' : 'buildParser';

		const par = `module.exports = ${ pegjs[ methodName ]( src, pegOptions ) };`;
		var fs = require( 'fs' );
		fs.writeFile( 'post-grammar-gb.js', par, function( err ) {
			if ( err ) {
				return console.log( err );
			}
			console.log( 'The file was saved!' );
		} );
		return par;
	},
};

The particular use of the pegjs options might be a key here. Though, I tried passing those same options to the postinstall script but it produced the same output anyway. Still, leaving the code above here in case it offers some other clue.

Contributor

hypest commented Jul 4, 2018

We need to replicate what you did in the past

OK, I think I found what I did in the past: I tapped into the test/unit/pegjs-transform.js code and added a "save to file" snippet. Here's what I think I used:

const pegjs = require( 'pegjs' );

module.exports = {
	process( src ) {
		// Description of PEG.js options: https://github.com/pegjs/pegjs#javascript-api
		const pegOptions = {
			output: 'source',
			cache: false,
			optimize: 'speed',
			trace: false,
		};
		const methodName = ( typeof pegjs.generate === 'function' ) ? 'generate' : 'buildParser';

		const par = `module.exports = ${ pegjs[ methodName ]( src, pegOptions ) };`;
		var fs = require( 'fs' );
		fs.writeFile( 'post-grammar-gb.js', par, function( err ) {
			if ( err ) {
				return console.log( err );
			}
			console.log( 'The file was saved!' );
		} );
		return par;
	},
};

The particular use of the pegjs options might be a key here. Though, I tried passing those same options to the postinstall script but it produced the same output anyway. Still, leaving the code above here in case it offers some other clue.

@gziolo gziolo modified the milestones: 3.2, 3.3 Jul 5, 2018

@hypest

This comment has been minimized.

Show comment
Hide comment
@hypest

hypest Jul 5, 2018

Contributor

False alarm @gziolo . The issue with the validation failing because < was appearing as &lt; is due to the not updated element package. So, the auto-generated parser works on my native mobile RN app tests just fine 👍.

Contributor

hypest commented Jul 5, 2018

False alarm @gziolo . The issue with the validation failing because < was appearing as &lt; is due to the not updated element package. So, the auto-generated parser works on my native mobile RN app tests just fine 👍.

@gziolo gziolo added this to In Progress in API freeze via automation Jul 10, 2018

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Jul 18, 2018

Member

I think I have another idea how to solve this for the @wordpress/blocks package. I will rather use prepublish script inside the package to ensure it gets published to npm properly.

Member

gziolo commented Jul 18, 2018

I think I have another idea how to solve this for the @wordpress/blocks package. I will rather use prepublish script inside the package to ensure it gets published to npm properly.

@gziolo gziolo closed this Jul 18, 2018

API freeze automation moved this from In Progress to Done Jul 18, 2018

@gziolo gziolo deleted the update/grammar-package branch Jul 18, 2018

@youknowriad

I personally like this change, while the postinstall trick is not perfect to build the grammar. We probably need support to custom build tools in our packages build script and also probably a bit harder support watching changes to the pegjs file as well. But I'm fine landing this as is because I think we'll be in a better position to make other packages generic (blocks, editor) which is far more important IMO

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Jul 18, 2018

Member

I will give it a spin again soon. I'm wrapping up PR for @wordpres/nux package 🎉

Member

gziolo commented Jul 18, 2018

I will give it a spin again soon. I'm wrapping up PR for @wordpres/nux package 🎉

@gziolo gziolo restored the update/grammar-package branch Jul 18, 2018

@gziolo gziolo reopened this Jul 18, 2018

API freeze automation moved this from Done to In Progress Jul 18, 2018

@gziolo gziolo changed the title from Packages: Create new grammar package to Packages: Create new `spec-parser` package Jul 18, 2018

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Jul 18, 2018

Member

@mcsf, @youknowriad, @hypest, @dmsnell it's ready for another check. Changes introduced:

  • Renamed to @wordpress/spec-parser as suggested by @dmsnell.
  • Readme file updated with more detailed description.
  • Added lerna run build step to ensure that the generated JS file gets updated every time we regenerate all packages. It should work with the watch mode.
  • Added simple unit tests to ensure that generated JS file works as expected.
  • RN version should work as is without overrides.
Member

gziolo commented Jul 18, 2018

@mcsf, @youknowriad, @hypest, @dmsnell it's ready for another check. Changes introduced:

  • Renamed to @wordpress/spec-parser as suggested by @dmsnell.
  • Readme file updated with more detailed description.
  • Added lerna run build step to ensure that the generated JS file gets updated every time we regenerate all packages. It should work with the watch mode.
  • Added simple unit tests to ensure that generated JS file works as expected.
  • RN version should work as is without overrides.

@gziolo gziolo requested a review from dmsnell Jul 18, 2018

@dmsnell

All good with me - thanks @gziolo

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Jul 18, 2018

Member

I still have some issues to make it work with Webpack. I'm not quite sure why.

Member

gziolo commented Jul 18, 2018

I still have some issues to make it work with Webpack. I'm not quite sure why.

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Jul 18, 2018

Member

b059cce updates build step for the package to use umd format to make it work both with node and browser.

Member

gziolo commented Jul 18, 2018

b059cce updates build step for the package to use umd format to make it work both with node and browser.

@aduth aduth modified the milestones: 3.3, 3.4 Jul 18, 2018

@@ -160,7 +160,10 @@ const config = {
rules: [
{
test: /\.js$/,
exclude: /node_modules/,
exclude: [
/block-serialization-spec-parser/,

This comment has been minimized.

@gziolo

gziolo Jul 19, 2018

Member

I have no idea how it worked locally with the previous setup 🤷‍♂️ I had to exclude this package from Babel transformation. Otherwise, it would get confused and try to treat this packages as harmony (import/export) module.

By they way, with our current setup, it looks like we should skip transpilation for all code inside packages folder as soon as we fully convert components into a package.

@gziolo

gziolo Jul 19, 2018

Member

I have no idea how it worked locally with the previous setup 🤷‍♂️ I had to exclude this package from Babel transformation. Otherwise, it would get confused and try to treat this packages as harmony (import/export) module.

By they way, with our current setup, it looks like we should skip transpilation for all code inside packages folder as soon as we fully convert components into a package.

This comment has been minimized.

@youknowriad

youknowriad Jul 19, 2018

Contributor

By they way, with our current setup, it looks like we should skip transpilation for all code inside packages folder as soon as we fully convert components into a package.

Yes, this should be done at some point, the blocker is that we need to extract the i18n strings into a unique file, we do so using a babel plugin. An alternative would be to run the generation of these strings per package and concat them on the root build.

@youknowriad

youknowriad Jul 19, 2018

Contributor

By they way, with our current setup, it looks like we should skip transpilation for all code inside packages folder as soon as we fully convert components into a package.

Yes, this should be done at some point, the blocker is that we need to extract the i18n strings into a unique file, we do so using a babel plugin. An alternative would be to run the generation of these strings per package and concat them on the root build.

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Jul 19, 2018

Member

I updated package name to @wordpress/block-serialization-spec-parser as suggested in comments.

It's ready for a final check.

Member

gziolo commented Jul 19, 2018

I updated package name to @wordpress/block-serialization-spec-parser as suggested in comments.

It's ready for a final check.

gutenberg_url( 'build/block-serialization-spec-parser/index.js' ),
array(),
filemtime( gutenberg_dir_path() . 'build/block-serialization-spec-parser/index.js' ),
true

This comment has been minimized.

@youknowriad

youknowriad Jul 19, 2018

Contributor

Do we really need this true in all registered scripts? I feel it's only necessary for edit-post?

@youknowriad

youknowriad Jul 19, 2018

Contributor

Do we really need this true in all registered scripts? I feel it's only necessary for edit-post?

This comment has been minimized.

@gziolo

gziolo Jul 19, 2018

Member

I think it fixes some issues, I remember seeing PR from @pento which added them everywhere.

@gziolo

gziolo Jul 19, 2018

Member

I think it fixes some issues, I remember seeing PR from @pento which added them everywhere.

@youknowriad

LGTM 👍

@gziolo gziolo merged commit 9b26aab into master Jul 19, 2018

2 checks passed

codecov/project 49.37% (+0.19%) compared to b06a335
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

API freeze automation moved this from In Progress to Done Jul 19, 2018

@gziolo gziolo deleted the update/grammar-package branch Jul 19, 2018

@gziolo gziolo referenced this pull request Jul 19, 2018

Closed

Improve Parser Performance #8044

@mcsf mcsf referenced this pull request Jul 27, 2018

Open

Overview of Short-term Parsing Enhancements #8244

3 of 11 tasks complete
@@ -2,5 +2,6 @@ build
build-module
coverage
node_modules
packages/block-serialization-spec-parser

This comment has been minimized.

@aduth

aduth Aug 16, 2018

Member

Why do we ignore the entire package? Could we just ignore the singular file(s) which are generated? The test file, for example, includes a number of legitimate code style violations.

@aduth

aduth Aug 16, 2018

Member

Why do we ignore the entire package? Could we just ignore the singular file(s) which are generated? The test file, for example, includes a number of legitimate code style violations.

This comment has been minimized.

@gziolo

gziolo Aug 20, 2018

Member

Right, this should be only one file. Let me fix it.

@gziolo

gziolo Aug 20, 2018

Member

Right, this should be only one file. Let me fix it.

This comment has been minimized.

@gziolo

gziolo Aug 20, 2018

Member

Haha, I know what happened. I used toMatchInlineSnapshot which uses prettier behind the scenes. We can't really use it with the current setup unless we adopt calypso-prettier :)

@gziolo

gziolo Aug 20, 2018

Member

Haha, I know what happened. I used toMatchInlineSnapshot which uses prettier behind the scenes. We can't really use it with the current setup unless we adopt calypso-prettier :)

This comment has been minimized.

@gziolo

gziolo Aug 20, 2018

Member

This should be addressed in #9166.

@gziolo

gziolo Aug 20, 2018

Member

This should be addressed in #9166.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment