Port from Batfish to Underreact #55

danswick · 2018-11-15T03:28:27Z

See https://github.com/mapbox/frontend-platform-team/issues/32. For sites like this one, which do not need to generate multiple HTML files, @mapbox/frontend-platform has a new favored tool: Underreact. It's significantly simpler than Batfish, and is using the latest, fastest versions of Webpack and Babel — so it's the better choice for any sites that don't need Batfish's HTML generation.

A few notes about the changes in this PR:

I used <link> and <script> tags in the HTML to load mbx-assembly.
The new app.js component is just the content of the old src/docs/app.js.
browserslist: ['defaults'] means Underreact won't bother building for IE11, because this is an internal site (that should speed things up).
Babel did not like the Object: type here and I'm not familiar enough with Babel to configure my way around that issue, so I just removed it.

Next steps:

Get the page to render 😬
Update tests

@davidtheclark: I would love to talk through this a bit with you tomorrow. I am a bit fuzzy on what's going on with the Babel 7 updates I made here and I am still not entirely sure why the page isn't being rendered.

danswick · 2018-11-16T04:33:48Z

Ok, after much flailing, I think this is ready to have its WIP tag removed. Tests are passing** and the page is rendering.

** tests are passing locally, but Travis is failing because it can't find @mapbox/underreact. @kepta: when I try to go to https://www.npmjs.com/login?next=/package/@mapbox/underreact/v/0.1.5 (or similar), I'm prompted to log in, which makes me think the package has recently become private.

kepta · 2018-11-16T14:32:47Z

** tests are passing locally, but Travis is failing because it can't find @mapbox/underreact. @kepta: when I try to go to https://www.npmjs.com/login?next=/package/@mapbox/underreact/v/0.1.5 (or similar), I'm prompted to log in, which makes me think the package has recently become private.

Looks like I have been mistakingly publishing it as private. I have publicly published it now and things should be working fine. Sorry about that.

davidtheclark · 2018-11-16T19:52:13Z

.gitignore

@@ -3,6 +3,7 @@ node_modules
 coverage
 .DS_Store
 _batfish*
+_underreact-site


Let's upgrade to the latest version of Underreact, in which case this will be _site.

davidtheclark · 2018-11-16T19:52:18Z

.gitignore

@@ -3,6 +3,7 @@ node_modules
 coverage
 .DS_Store
 _batfish*


Can we delete this line?

davidtheclark · 2018-11-16T19:53:33Z

src/index.js

@@ -0,0 +1,8 @@
+import React from 'react';


If this file is specific to docs, lets put it in src/docs.

davidtheclark · 2018-11-16T19:53:55Z

underreact.config.js

@@ -0,0 +1,11 @@
+'use strict';


Is this for the docs or the test cases? Let's run both with Underreact.

@davidtheclark I'm not totally clear on how we would use Underreact for both the docs and the test-cases app in the same project.

Have you looked at how each app works now, without Underreact? That would be the place to start.

Underreact commands have a --config option that you can use in order to store two different configs in one repo and run them in different Underreact processes.

To give an example here is a package.json using an explicit config:

https://github.com/mapbox/streets-review-frontend/blob/fb35b1ad05da326ee20a125e6599569c2f7d0cf2/package.json#L27

…erreact

kepta · 2018-11-19T14:12:57Z

Looks like this might be a great place to enable hot reloading feature of Underreact 🔥. We can either add it in this PR or I can send another PR to specifically enable it.

danswick · 2018-11-21T02:47:22Z

I've done some reorganizing here that I hope will make the project easier to maintain:

Moved docs and the test-cases-app into their own directories separate from the source components.
Finished scaffolding for the docs page's port to underreact.
Begain porting the test-cases-app to underreact, but hit a dead-end trying to get it working (and also got myself all kinds of turned around trying to get eslint configured). Underreact seems to be building the page without issue and my React chrome dev tools indicate that the test cases component and ReactTestKitchen component are being created, but they're not being populated with anything and are not throwing any errors. I'm a bit lost on where things are going wrong and could use some backup if anyone from @mapbox/frontend-platform is wiling to take a look with me.

kepta · 2018-11-21T09:36:30Z

@danswick this was an interesting bug. I found that the bug is with the way we are using the basePath as '/mr-ui/test-cases-app', but the react-test-kitchen expects the basePath to be / and that's why the client side routing in rtk was wasn't outputting anything.

https://github.com/mapbox/react-test-kitchen/blob/1dbc8fb4e33c86cd02b66bd3f077587abe287a42/src/react-test-kitchen.js#L19-L23

We can probably send a fix to rtk to understand base paths or you can simply use / to keep it happy 😅.

Some notes on the bug hunt 👨‍🚒 :

The way we are auto-generating the files is pretty confusing to me (Maybe it is just me being unfamiliar with the rtk testing pattern).
Also, I wasn't careful to see that budo server is also being used and the fact that you reorganised the repo structure made the auto scripts generation broken.
If it is not too much of a trouble, I suggest saving the repo structure change to a separate issue/PR. This would make it easier to grok at the git diff and also keep only changes relevant to porting of Underreact in this PR.

davidtheclark · 2018-11-21T14:56:54Z

I suggest saving the repo structure change to a separate issue/PR

Yes, definitely.

danswick · 2018-11-21T16:25:55Z

Wow, thanks for tracking that down, @kepta! I can reorganize the structure for sure. The reason I changed it in the first place was that I found it confusing to have two web apps that reference the component source, but the component source lives alongside just one of the apps. So I separated everything just to help conceptualize which parts were interacting with what. I can totally see how the diff would be hard to make sense of in this state.

davidtheclark · 2018-11-21T17:11:55Z

I found it confusing to have two web apps that reference the component source, but the component source lives alongside just one of the apps

I don't think I understand the confusion here. We have src/components, src/docs, and src/test-cases, so the source components are already alongside both apps.

danswick · 2018-11-30T00:30:03Z

Thanks for all of the feedback, @kepta and @davidtheclark. I went ahead and fixed up the project structure, swapped the test cases app to a / basepath (thanks for that hint, @kepta! side note: I started experimenting with configurable basepath in rtk here, but was having some trouble and opted to stick with a basepath of / since the test cases app is really only ever used locally), and updated snapshots to get tests passing. Would love another review now that the diff is more manageable 🙏

davidtheclark

Fantastic, @danswick. I tried it out locally and had no problems at all. Seems like you got it 👍 🍕

davidtheclark · 2018-11-30T15:35:22Z

.artifacts.yml

@@ -0,0 +1,3 @@
+version: v1
+defaults:
+  - publisher


Mr UI isn't currently being published with Publisher, right?

Oh yowza. I just got into Underreact robot mode and put this here automatically. I can remove.

davidtheclark · 2018-11-30T15:35:42Z

.eslintignore

@@ -6,3 +6,5 @@ src/docs/data
 pkg
 src/test-cases-app/component-index.js
 dist
+*underreact.config.js


Should this be **/underreact.config.js?

It works this way, but **/*underreact.config.js is more precise.

davidtheclark · 2018-11-30T15:36:16Z

.eslintignore

@@ -6,3 +6,5 @@ src/docs/data
 pkg
 src/test-cases-app/component-index.js
 dist
+*underreact.config.js
+html.js


Hm, we shouldn't need to do this. But I don't want to clog up the PR for linting. Let's sort it out after the merge. (Hint: I think we'll use ESLint's overrides option.)

Sounds good 👍

davidtheclark · 2018-11-30T15:37:03Z

scripts/build-module-indexes.js

@@ -5,9 +5,8 @@ require('hard-rejection/register');
 const fs = require('fs');
 const path = require('path');
 const pify = require('pify');
-


davidtheclark · 2018-11-30T15:37:30Z

scripts/build-module-indexes.js

 const excludeDirs = new Set(['utils']);
-const srcRoot = path.resolve(__dirname, '../src/components');
+const srcRoot = path.resolve(__dirname, '../src/components/');


👹

(This change shouldn't make any difference, right?)

davidtheclark · 2018-11-30T15:44:25Z

src/docs/docs-underreact.config.js

+
+module.exports = () => {
+  return {
+    siteBasePath: '/mr-ui/docs',


The true base path is just /mr-ui, right? https://mapbox.github.io/mr-ui/

Ah, yep! This is a gremlin from when I was trying to give docs and test-cases-app their own base paths under mr-ui 😬

davidtheclark · 2018-11-30T15:45:24Z

src/test-cases-app/html.js

+    <meta charset='utf-8'>
+    <meta name='viewport' content='width=device-width, initial-scale=1'>
+    <link href="https://api.mapbox.com/mapbox-assembly/mbx/v0.26.0/assembly.min.css" rel="stylesheet">
+    <script async src="https://api.mapbox.com/mapbox-assembly/mbx/v0.26.0/assembly.js"></script>


🤔 it's suspicious to me that we have different version of Assembly in the test cases and docs. Are you copying a mistake already in place, or is this a typo?

Looks like it was already in place, but I just bumped them both up to 0.28.1 and tested locally and everything looks good 👍

begin porting to underreact

6e6bb10

danswick requested a review from davidtheclark November 15, 2018 03:28

danswick added 4 commits November 15, 2018 08:19

small update

cd82f9a

fix html

9b6f647

update snapshots

561f7d0

underreact version

300f9fe

danswick changed the title ~~[WIP] Port from Batfish to Underreact~~ Port from Batfish to Underreact Nov 16, 2018

kepta added 2 commits November 16, 2018 20:07

fix underreact version

1567f09

Merge branch 'underreact' of github.com:mapbox/mr-ui into underreact

1f27cfd

kepta force-pushed the underreact branch from 7cbba9b to 1f27cfd Compare November 16, 2018 15:48

davidtheclark reviewed Nov 16, 2018

View reviewed changes

danswick added 2 commits November 16, 2018 14:27

update to underreact 0.3

1c73edb

Merge branch 'underreact' of https://github.com/mapbox/mr-ui into und…

98b9ce7

…erreact

danswick mentioned this pull request Nov 20, 2018

Implement Underreact's new hot-reload feature #58

Closed

danswick added 2 commits November 20, 2018 18:15

reorganize for double-underreact

d834c40

more underreact reorganization

eb57d05

danswick added 2 commits November 29, 2018 16:22

move back to orig structure and do not set test-cases basepath

b94a40f

Merge branch 'master' into underreact

f07be86

davidtheclark approved these changes Nov 30, 2018

View reviewed changes

address feedback

ac5d76a

danswick merged commit 0b06b39 into master Nov 30, 2018

danswick deleted the underreact branch November 30, 2018 21:29

danswick mentioned this pull request Dec 19, 2018

Update babel things for package build and prep for 0.4.0 release #67

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port from Batfish to Underreact #55

Port from Batfish to Underreact #55

danswick commented Nov 15, 2018 •

edited

danswick commented Nov 16, 2018

kepta commented Nov 16, 2018

davidtheclark Nov 16, 2018

davidtheclark Nov 16, 2018

davidtheclark Nov 16, 2018

davidtheclark Nov 16, 2018

danswick Nov 20, 2018

davidtheclark Nov 20, 2018

kepta Nov 20, 2018

kepta commented Nov 19, 2018

danswick commented Nov 21, 2018

kepta commented Nov 21, 2018 •

edited

davidtheclark commented Nov 21, 2018

danswick commented Nov 21, 2018

davidtheclark commented Nov 21, 2018

danswick commented Nov 30, 2018

davidtheclark left a comment

davidtheclark Nov 30, 2018

danswick Nov 30, 2018

davidtheclark Nov 30, 2018

danswick Nov 30, 2018

davidtheclark Nov 30, 2018

danswick Nov 30, 2018

davidtheclark Nov 30, 2018

davidtheclark Nov 30, 2018

danswick Nov 30, 2018

davidtheclark Nov 30, 2018

danswick Nov 30, 2018

davidtheclark Nov 30, 2018

danswick Nov 30, 2018

Port from Batfish to Underreact #55

Port from Batfish to Underreact #55

Conversation

danswick commented Nov 15, 2018 • edited

danswick commented Nov 16, 2018

kepta commented Nov 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kepta commented Nov 19, 2018

danswick commented Nov 21, 2018

kepta commented Nov 21, 2018 • edited

davidtheclark commented Nov 21, 2018

danswick commented Nov 21, 2018

davidtheclark commented Nov 21, 2018

danswick commented Nov 30, 2018

davidtheclark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danswick commented Nov 15, 2018 •

edited

kepta commented Nov 21, 2018 •

edited