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

Use the GB parser pipeline #45

Merged
merged 13 commits into from
Jul 2, 2018
Merged

Use the GB parser pipeline #45

merged 13 commits into from
Jul 2, 2018

Conversation

hypest
Copy link
Contributor

@hypest hypest commented May 24, 2018

This PR introduces the usage of the Gutenberg grammar + block attributes parser.

Instead of having the hardcoded blocks list be constructed by manually calling createBlock(), this PR uses the GB Parser to construct the blocks from html snippets, just like those will be found on a real post.

@hypest hypest requested a review from gziolo May 24, 2018 13:41
@gziolo
Copy link
Contributor

gziolo commented May 30, 2018

src/store/post-grammar-gb.js - why this file is required?

In Gutenberg we import it using import { parse as grammarParse } from './post.pegjs';. Did you try to replicate Webpack loader logic to handle it?

@gziolo
Copy link
Contributor

gziolo commented May 30, 2018

I need to setup everything locally and spend some time investigating jsdom implementation to give more detailed feedback. The general direction taken here makes sense. I'm wondering if we can find a better way to load grammar similar to how it is done for web.

@hypest
Copy link
Contributor Author

hypest commented May 31, 2018

Thanks for having a look @gziolo !

src/store/post-grammar-gb.js - why this file is required?

In Gutenberg we import it using import { parse as grammarParse } from './post.pegjs';. Did you try to replicate Webpack loader logic to handle it?

I did give it a try utilizing babel and the transformation steps in Metro but had no success in the time frame I devoted to it. To move things forward I resorted to the "prebuilt" grammar parser. Besides, we can always try again in the future, and we should to make sure the parser stays up-to-date.

I need to setup everything locally and spend some time investigating jsdom implementation to give more detailed feedback.

Nice! Feel free to ping me to give a hand.

The general direction taken here makes sense.

Good to know! Let's wait for the extra pass before we merge, after you try it locally. Thanks! 🙇

@gziolo
Copy link
Contributor

gziolo commented May 31, 2018

I did give it a try utilizing babel and the transformation steps in Metro but had no success in the time frame I devoted to it. To move things forward I resorted to the "prebuilt" grammar parser. Besides, we can always try again in the future, and we should to make sure the parser stays up-to-date.

Fair enough, it's okey as a temporary solution, but we should try to bring it closer to what web does in the near future. I hope we will tackle it somehow as part of publishing blocks to npm. Not sure, it seems to be a blocker for npm package and a very tricky one :)

@hypest
Copy link
Contributor Author

hypest commented May 31, 2018

Not sure, it seems to be a blocker for npm package

Oh, you mean we cannot package and publish the src/store/post-grammar-gb.js file atm?

@gziolo
Copy link
Contributor

gziolo commented May 31, 2018

Not sure, it seems to be a blocker for npm package

Oh, you mean we cannot package and publish the src/store/post-grammar-gb.js file atm?

Webpack does it atm, so not quite sure how this fits into the whole flow. Distribution files for packages are generated with Babel.

@hypest
Copy link
Contributor Author

hypest commented Jun 25, 2018

Fixed conflicts with base branch with f8d875f.

@gziolo
Copy link
Contributor

gziolo commented Jul 2, 2018

Testing steps:

yarn clean
git submodule update
yarn
yarn start
yarn ios or yarn android

@gziolo
Copy link
Contributor

gziolo commented Jul 2, 2018

src/store/post-grammar-gb.js can be removed. We use its copy inside blocks module.

Otherwise, everything looks and works as expected. Let's focus on bringing a compiled version of the parser inside Gutenberg.

Copy link
Contributor

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

🚢

At gutenberg/blocks/api/post-grammar-parser-pre-generated.js
@hypest
Copy link
Contributor Author

hypest commented Jul 2, 2018

src/store/post-grammar-gb.js can be removed. We use its copy inside blocks module.

Done!

Thanks for the review @gziolo , will merge now!

@hypest hypest merged commit d73c443 into master Jul 2, 2018
@hypest hypest deleted the feature/parser branch July 2, 2018 11:23
@hypest hypest mentioned this pull request Aug 13, 2018
hypest pushed a commit that referenced this pull request Nov 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants