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

Expose the grammar parser to the mobile app #7691

Merged
merged 10 commits into from Jul 6, 2018

Conversation

Projects
None yet
2 participants
@hypest
Contributor

hypest commented Jul 3, 2018

Description

Expose the parse function from the parser module to the native mobile app variant of the Block Api.

How has this been tested?

Besides successfully running npm test and npm run dev, tested this using this PR from the native app repo.

Types of changes

  • Adds a new feature, exposing the parser to the native app
  • Splits the way the parser is loaded to allow the pegjs parser (sourced and transformed via webpack) to be loaded for the web while a pre-generated parser is loaded on the native mobile since the transformation machinery is not yet available there
  • Adds the RawHTML function to the element package for the native mobile

Checklist:

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

hypest added some commits May 17, 2018

Offer RawHTML to fix warning during save()
Warning is:
"Warning: React.createElement: type is invalid -- expected a string (for
built-in components) or a class/function (for composite components) but
got: undefined."
@hypest

This comment has been minimized.

Contributor

hypest commented Jul 3, 2018

Any ideas on how to improve the testcov on this one? Not sure how to add tests for the native mobile code here.

*
* @return {WPElement} Dangerously-rendering element.
*/
export function RawHTML( { children, ...props } ) {

This comment has been minimized.

@gziolo

gziolo Jul 4, 2018

Member

Can we move this component to its own file (raw-hatml.js?) and reference it from both index files to avoid code duplication and possible sync issues in the future?

This comment has been minimized.

@hypest

hypest Jul 4, 2018

Contributor

Good call @gziolo , made the change with da43e07.

I chose the rather boring name index-common.js for the base module, also having in mind that more functions will most probably end up there as we expose them to the native app. Let me know if you think the name can be a problem.

@gziolo

This comment has been minimized.

Member

gziolo commented Jul 4, 2018

Any ideas on how to improve the testcov on this one? Not sure how to add tests for the native mobile code here.

We need to decide what to do about #7664, where I proposed moving the parser and grammar to their own package and skip Webpack loader altogether. This will solve the issue with the code coverage, but more importantly, it will make this module to work independently to Webpack config when its published to npm.

@hypest

This comment has been minimized.

Contributor

hypest commented Jul 4, 2018

We need to decide what to do about #7664,

Right. I think that ideally, we want to have a simple and robust way to expose the parser to both the web and the native mobile app. So, #7664 in on the right path, for many reasons.

That said, it's not clear to me yet how soon we'll get there so, this PR with its pre-generated parser is a solution that helps unblock work on the native mobile side.

I've already added tests for the pre-generated parser so, if it's OK with you and provided #7664 doesn't get merged this week, I'd prefer if we merge this PR and optimize on top of it. Let me know what you think @gziolo .

*
* @return {Array} Block list.
*/
export function parsePreGenerated( content ) {

This comment has been minimized.

@gziolo

gziolo Jul 5, 2018

Member

We can use the following code to avoid duplication in the blocks/api/parsers.js:

/**
 * Creates a parse implementation for the post content which returns a list of blocks.
 *
 * @param {Function} parseImplementation Parse implementation.
 *
 * @return {Function} An implementation which parses the post content.
 */
export const createParse = ( parseImplementation ) =>
    ( content ) => parseImplementation( content ).reduce( ( memo, blockNode ) => {
        const block = createBlockWithFallback( blockNode );
        if ( block ) {
            memo.push( block );
        }
        return memo;
    }, [] );

/**
 * Parses the post content with a PegJS grammar and returns a list of blocks.
 *
 * @param {string} content The post content.
 *
 * @return {Array} Block list.
 */
export const parseWithGrammar = createParse( grammarParse );

This comment has been minimized.

@hypest

hypest Jul 5, 2018

Contributor

Thanks for the proposal @gziolo ! Made the change in ca2a9ec.

type: 'string',
source: 'text',
// run the test cases using the PegJS defined parser
testCases( parsePegjs );

This comment has been minimized.

@gziolo

gziolo Jul 5, 2018

Member

Can we put those test cases in their own describe blocks to make it easier to debug which version of the parser failed?

This comment has been minimized.

@hypest

hypest Jul 5, 2018

Contributor

Separated the 2 run in different describe() blocks with 2a012ff.

@@ -0,0 +1,20 @@
import { createElement } from 'react';

This comment has been minimized.

@gziolo

gziolo Jul 5, 2018

Member

As a convention, we add dependencies blocks for import statements, in this case it will be:

/**
 * External dependencies
 */

This comment has been minimized.

@hypest

hypest Jul 5, 2018

Contributor

Ah, missed that, sorry :(. Fixed with 9ace658.

@hypest

This comment has been minimized.

Contributor

hypest commented Jul 5, 2018

Addressed the new review @gziolo , ready for another pass, thanks!

@gziolo

gziolo approved these changes Jul 6, 2018

All good, thanks for applying all those changes.

@gziolo gziolo merged commit 87252f2 into master Jul 6, 2018

2 checks passed

codecov/project 48.41% (+1.98%) compared to 48914ec
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gziolo gziolo deleted the rnmobile/parser-updated-master branch Jul 6, 2018

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