Point to the GB packages src folder by hypest · Pull Request #66 · wordpress-mobile/gutenberg-mobile

hypest · 2018-07-09T13:37:31Z

This PR changes the way the Gutenberg packages are consumed by removing the need to compile (npm install) the packages first, introduced here.

See this convo for some additional background on the "why".

Changes:

On the GB side, add the react-native package.json entry point and have it point to the src/index module (notice the lack of the .js extension, that's on purpose to let Metro do its smart resolve)
Configure Jest to point directly to the src/index module as well
Add some more transformations to be performed by Babel, to support async/await. See this convo for some background.

To test:

Clean the GB folder from the compiled artefacts. A handy way to do that is git clean -fdX which deletes the non-tracked files/folders.
Run the RN app and it should compile and run correctly

In order to consume GB packages directly from their src (instead of precompiled file in `build`), we need to enable more babel transofrmations.

Blacklisting the specific packages folders we want Metro to resolve from NPM, instead of automagically resolving them to local.

mzorz · 2018-07-10T12:18:27Z

Thanks for the steps provided @hypest ! I was able to make it run after going through these provided steps, copying here for completeness:

git clone git@github.com:wordpress-mobile/gutenberg-mobile.git
cd gutenberg/
git checkout feature/direct-packages-src
git submodule init
git submodule update
yarn install
yarn clean:runtime && yarn start --reset-cache <- using the new script to clear caches (clean:runtime) along with the usual --reset-cache
and in another terminal window: yarn android

mzorz

@hypest ! I’m asking a ton of questions many of them will be annoying… as I'm asking to explain a lot. I’m asking these things as they come to mind but please feel free to answer in a way that leads us to “understand just enough” to continue working on stuff and not affect the team's speed negatively.
Thanks in advance!

mzorz · 2018-07-10T12:28:19Z

.babelrc

      }
    ],
+    "transform-async-to-generator",
+    "transform-async-generator-functions",


I don't understand why these two are needed, can you provide an explanation to it please? In the PR description I can read:
Add some more transformations to be performed by Babel, to support async/await

But still, it's not evident to me where async/await is being used. Is this in preparation for an upcoming PR, maybe?

Good question! Here's what happens: by trying to use the src version of the files in the GB packages, we by pass the step where the GB build transpiles them to ES5. So, we need to help Babel compile those for us.

The async/await usage is actually in the GB packages, not the RN app. See https://github.com/WordPress/gutenberg/blob/ccbb54b4de3f0c471fa938faecb6e52c166f3991/packages/data/src/index.js#L175. For example, comment out the "transform-async-generator-functions" line in our Babel configuration and see the error that is emitted. It will be in code inside the GB data package.

Let me know if this answers your question. 👍

Thanks, I'd not find that myself other than by commenting that out as suggested

mzorz · 2018-07-10T12:29:56Z

.travis/travis-checks-js.sh

-  pushd gutenberg
-  npm i || pFail
-  popd
-


Please remember to remove the steps from README.md as well

Good catch! Will do it now. Edit: done with be0d2de.

mzorz · 2018-07-10T12:32:22Z

jest.config.js

 	moduleNameMapper: {
 		'@wordpress\\/(blocks|editor)$': '<rootDir>/gutenberg/$1',
-		'@wordpress\\/(data|element|deprecated)$': '<rootDir>/gutenberg/packages/$1',
+		'@wordpress\\/(data|element|deprecated)$': '<rootDir>/gutenberg/packages/$1/src/index',


I believe this is the key to this PR - could you explain how this would be benefitial, with an example, maybe? Otherwise I can only assume you're going to be using this elsewhere (it's not apparent from this PR itself, what the value it may add is)

Also, this is the jest configuration, but do we have a place where it's made clear and apparent that the main configuration uses the source files?

I believe this is the key to this PR

Not quite key per se, for 2 reasons:

It modifies how the packages are resolved (overwritten in this case) for the tests (Jest), not the app code

It's basically a workaround because Jest resolves the entry point of a package by reading the main field in the package.json. For example, for the element package the entry point is the built file. See here on the GB repo. In an ideal world, the Jest tests would use the same entrypoint as the RN app so we wouldn't need to configure Jest separately.

A question one could raise here is: why do we even want to use the package source files instead of the already transpiled ones? The answer is that we prolly want to do that for the near future so we can iterate fast on the packages themselves. For example, with the current PR, one can modify the package src/index.js and have the change be picked up immediately by the RN app. With the built files, one would need to manually go through npm install to rebuild the packages before using the change.

I expect that in the not-so-distant future, we will revert back to using the built files after the project matures enough and we only introduce changes to the packages rarely.

do we have a place where it's made clear and apparent that the main configuration uses the source files?

Yes, it's the react-native entry point, here for example on the GB repo.

Not quite key per se, for 2 reasons:

Right, I realized it was just for jest in my second comment :) - thanks for pointing to the main, module and react-native keys in package.json, these values made sense 👍

This paragraph here is 🏅 :

A question one could raise here is: why do we even want to use the package source files instead of the already transpiled ones? The answer is that we prolly want to do that for the near future so we can iterate fast on the packages themselves. For example, with the current PR, one can modify the package src/index.js and have the change be picked up immediately by the RN app. With the built files, one would need to manually go through npm install to rebuild the packages before using the change.

This is probably what I'd find useful the most in the PR description to understand the purpose of the PR itself - maybe a suggestion for future PRs 🙇 (thank you very much for the clarification)

mzorz · 2018-07-10T12:35:14Z

src/store/index.js


 const devToolsEnhancer =
-	( 'development' === process.env.NODE_ENV && require( 'remote-redux-devtools' ).default ) ||
+	// ( 'development' === process.env.NODE_ENV && require( 'remote-redux-devtools' ).default ) ||


If this is not needed, let's remove this line. Otherwise, if this is something you'll need for later, please let's add a TODO comment here, explaining what and why succinctly.

We don't need it but the remote-redux-devtools offers a very nice debugger for Redux. I had to comment it out because it's not super important at the moment and I couldn't make it work in this PR in the short amount of time I was willing to devote to it. I left it in as a comment only as a reminder to try reinstate it in the future. Perhaps marking it with "TODO:" can help there.

mzorz · 2018-07-10T12:39:26Z

rn-cli.config.js

+	getBlacklistRE: function() {
+		// blacklist the GB packages we want to consume from NPM (online) directly
+		return blacklist( [ /gutenberg\/packages\/(autop|hooks|i18n|is-shallow-equal)\/.*/ ] );
+	},


Again here - I got more context from a chat elsewhere about this being denylisted as we think we won't need to modify source files for. That makes a lot of sense, but just by reading code alone (not even this PR, but the code in a general way) it's difficult to understand that we are using the source files and denylisting a subset of modules for the purpose of ease at modifying while developing. Can we add a nice comment in code that connects this part and <wherever it's indicated in code we're using source files>?

Fair point. The packages we currently load from source have been chosen a bit arbitrarily, except for the element package in which we already have native code and so it's more likely we'll want to modify-and-try out fast. The other packages we use from source (data and deprecated) don't have native code and so, I just went ahead and made them resolved from NPM (with 8e5ebc8).

Can we add a nice comment in code that connects this part and <wherever it's indicated in code we're using source files>?

I added a relevant comment (with 08f3626) but notice that effectively, there's nowhere something indicating that a package is loaded directly from source. What happens is that Metro automagically figures out that a module imported in code (example) can be satisfied by the package found inside the gutenberg/packages/<module> folder.

Excellent, thanks for this answer, I think it makes it clearer 👍

This reverts commit ecb5baf.

hypest · 2018-07-10T14:23:07Z

Thanks for the review and the detailed questions @mzorz ! I tried to answer them but please, let me know if anything still doesn't make good sense.

mzorz · 2018-07-10T15:32:43Z

Thanks for your thorough answers @hypest ! Having tested and read, I've nothing to add except LGTM

hypest · 2018-07-10T15:43:22Z

Thanks for merging @mzorz ! I amended the PR description, adding links to the conversations we had in the comments, to help the reader understand the changes.

Among other things, I can see how the short PR description (removing the need to compile (npm install) the packages first) can get lost in the sea of unfamiliar notions, intricacies and workarounds. It's not always easy to figure out what's known or not so, even as part of the PR review, your questions that asked for elaborating were :gold:. Keep'em coming 😃 🙇‍♂️

…api-level Feature/match wpandroid api level

hypest added 5 commits July 9, 2018 14:22

Enable babel async transforms to consume GB packages

7bcc0e8

In order to consume GB packages directly from their src (instead of precompiled file in `build`), we need to enable more babel transofrmations.

remote-redux-devtools causes problems, disable for now

bc50e4d

Point Jest to the packages/src/index module

44a8167

No need to compile GB anymore

a572c9e

equivalent-key-map is a dev dependency

4127966

hypest added the [Type] Enhancement Improves a current area of the editor label Jul 9, 2018

hypest requested a review from mzorz July 9, 2018 13:37

hypest mentioned this pull request Jul 9, 2018

Point the package entry to src directly for native mobile WordPress/gutenberg#7826

Merged

4 tasks

hypest added 2 commits July 10, 2018 11:00

Use the GB master now that GB branch got merged

2bf678f

Prevent Metro from picking up some packages from local

5c93af9

Blacklisting the specific packages folders we want Metro to resolve from NPM, instead of automagically resolving them to local.

mzorz reviewed Jul 10, 2018

View reviewed changes

hypest added 3 commits July 10, 2018 16:17

Revert "Update README to note that GB needs its npm install"

be0d2de

This reverts commit ecb5baf.

No native code so, load GB's data, deprecated from NPM

8e5ebc8

Comment about no list of loaded-from-source GB packages

08f3626

mzorz merged commit f7cb58f into master Jul 10, 2018

mzorz deleted the feature/direct-packages-src branch July 10, 2018 15:33

This was referenced Jul 10, 2018

#66 broke master with "undefined is not a valid argument for 'in' (evaluating 'styleProp in style')" #67

Closed

Load the data package from local GB source tree #68

Merged

hypest added a commit that referenced this pull request Nov 2, 2018

Merge pull request #66 from wordpress-mobile/feature/match-wpandroid-…

9d96fff

…api-level Feature/match wpandroid api level

Conversation

hypest commented Jul 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mzorz commented Jul 10, 2018

Uh oh!

mzorz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hypest Jul 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mzorz Jul 10, 2018 • edited by hypest Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hypest commented Jul 10, 2018

Uh oh!

mzorz commented Jul 10, 2018

Uh oh!

hypest commented Jul 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hypest commented Jul 9, 2018 •

edited

Loading

hypest Jul 10, 2018 •

edited

Loading

mzorz Jul 10, 2018 •

edited by hypest

Loading