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 latest gb master as per July 2nd, 2018 #61

Merged
merged 17 commits into from
Jul 6, 2018

Conversation

hypest
Copy link
Contributor

@hypest hypest commented Jun 28, 2018

This PR refreshes the reference to GB's master.

⚠️Headsup: this PR adds an new setup step: manulally install the Gutenberg packages (npm install inside the GB directory.

Added a couple of yarn scripts (start:reset, clean:runtime, clean:setup) to help modularize and provide some shortcuts to frequently used CLI commands.

hypest added 4 commits June 28, 2018 12:56
That's the folder where the source code of the published NPM packages
resides and it's only used for the publishing to NPM process. We need to
use the published packages from NPM in the first place.
@hypest
Copy link
Contributor Author

hypest commented Jun 28, 2018

ATM, the build (tests) fail because the GB package babel-plugin-import-jsx-pragma doesn't get resolved correctly. That package is consumed in the GB's Babel configuration directly from the gutenberg/packages folder and we might need to find a workaround for that until the package is published to NPM normally like the rest of the packages in that folder.

@gziolo gziolo changed the title Use latest gb master as per June 28th, 2018 Use latest gb master as per July 2nd, 2018 Jul 2, 2018
hypest added 2 commits July 4, 2018 00:56
Also added a `preset` directive to the "fake" .babelrc file since that's
needed for proper transformation of the ES6 files before they get
executed in Jest.
@hypest
Copy link
Contributor Author

hypest commented Jul 3, 2018

Fixed the issue with the GB Babel configuration having a local hardcoded path to babel-plugin-import-jsx-pragma by employing the "bypassing" .babelrc file trick.

The Travis build now fails because of some issue with the shell commands around the .babelrc trick. Those shell commands succeed locally so this must be due to some mismatch with the shell running in the Travis VM. I've contacted Travis support to help.

Even after that shell issue gets fixed though (Edit: fixed with 254446c), there's an additional issue that will make the testsuite fail: Block validation fails for the more block.
That happens because the @wordpress/element package entrypoint (main in its package.json) is pointing to the index.js file (including its file .js extension) which shortcircuits the Metro resolving logic, failing to pick up the native index.native.js of the package. That's an issue that WordPress/gutenberg#7675 would solve but in there we opted for introducing the react-native entrypoint instead, as a safer choice. Under the new light where the Jest tests fail here we will need to reconsider a solution. /cc @gziolo

hypest added 7 commits July 4, 2018 10:57
Edit the files in the `build/` subfolders directly to have hot-reload
work. If you change the ones in `src/` then the packages need to be
rebuilt via GB (npm run build:packages).

* Metro does its magic here and finds the packages directly in the
gutenberg/packages folder automagically
* Jest needs to be configured to find them (mapping)
* Flow needs configuration too
Those might be present if you build GB.
@hypest hypest force-pushed the feature/use-latest-gb-master-june292018 branch from cb62dca to 6f909f9 Compare July 6, 2018 00:44
@hypest
Copy link
Contributor Author

hypest commented Jul 6, 2018

I've changed (aac9681) the source from where the GB packages are consumed to point directly to the GB packages folder. This has these effects:

  • The latest packages can be used directly while developing, plus we can quickly do/test any modifications we need in them. No need to publish the packages to NPM before using them.
  • The GB packages need to be built (npm i in the GB directory) which takes a bit of time, especially the first time. I left this step as a manual one until we find an efficient way to automate it via a script.
  • Modifications to the GB package sources need to be done directly in the built files (e.g. the ones in gutenberg/packages/element/build/*) for the changes to be picked up by the app (live-reload works too). If the ones in src/ are modified, that will require a npm i to build them.

Also, updated the GB reference to point to 2a012ff which includes the element package fixes (see WordPress/gutenberg#7675 and WordPress/gutenberg#7691).

With all the changes above the build is now green on Travis.

The Gutenberg packages need to be installed and built. This is done via the following commands:

```
pushd gutenberg
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we put it in the shell script and run as postinstall script for the repository?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps. I refrained from automating it from the beginning so to give us time/space to see how we use it actually but, I can give it a try now and see.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, gave postinstall. Our patterns these days involve more than a few yarn install attempts and that will cause npm i to be called too many times, unnecessarily. Ideally, we want npm i to be called rarely, like, when the GB repo is cloned or when the GB branch is changed.

I think we can leave npm i out of automation for the following days/weeks and re-assess, depending on the patterns we notice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And btw, thanks for the review @gziolo !

@@ -6547,6 +6547,10 @@ remark-parse@4.0.0:
vfile-location "^2.0.0"
xtend "^4.0.1"

rememo@^3.0.0:
Copy link
Contributor

Choose a reason for hiding this comment

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

The only downside of the current setup is that we need to ensure that all npm packages are referenced in package.json in the main repository folder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, thankfully, the ones missing get reported by Metro when yarn start the packager so it's rather easy to spot.

@daniloercoli
Copy link
Contributor

Looks good! :shipit:

@daniloercoli daniloercoli merged commit eee43c8 into master Jul 6, 2018
@daniloercoli daniloercoli deleted the feature/use-latest-gb-master-june292018 branch July 6, 2018 10:19
hypest pushed a commit that referenced this pull request Nov 2, 2018
…ion-absolute-position

Android Fix - cursor position - Return the exact location of the cursor in the editor
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.

3 participants