aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David McLaughlin <da...@dmclaughlin.com>
Subject Re: Review Request 62135: HomePage implemented in Preact
Date Mon, 11 Sep 2017 17:13:55 GMT


> On Sept. 11, 2017, 4:18 p.m., Joshua Cohen wrote:
> > lgtm overall. One question: do you think it's worthwhile to extract the shallow
render to its own npm module that we depend on? Maybe that would make it possible/easier to
have it upstreamed to preact? No idea if there are any challenges to that from an OSS perspective
(i.e. we're ok to contribute to Aurora, but creating a new OSS project/publishing to npm/etc.
may need an internal OSS review at Twitter?).

I think there's probably better ways to do it if I had the time to learn the internals of
Preact. The major downside to this approach is that it's comparing a rendered DOM tree to
a vnode tree. The way it's done in React is *much* cleaner, where you're comparing vnode tree
to vnode tree and can just use basic equality and tree traversal techniques. 

One of the goals of Preact is to be super lightweight, and for that reason they couple their
rendering directly to the DOM, which is why the shallow rendering here plugs into that.  There
is a String renderer (https://github.com/developit/preact-render-to-string) that exposes a
shallowRender method, but you end up comparing properties like Object[object object] when
you have complex properties and I ended up with a lot of false positive tests. I haven't been
able to figure out how much it would take to create a vnode render, but I was put off by the
TODO in the preact-test-utils library (https://github.com/windyGex/preact-test-utils/blob/master/src/index.js#L6).
The author implies in one of the github issuess that implementing shallow rendering is very
complex in Preact. I'm going to keep an eye out though in the hopes that someone steps up
and does it cleanly there.. that library is the basis for enzyme compatibility. If I ever
have time, I may even take a crack at it myself!


> On Sept. 11, 2017, 4:18 p.m., Joshua Cohen wrote:
> > ui/src/main/js/components/Breadcrumb.js
> > Lines 12 (patched)
> > <https://reviews.apache.org/r/62135/diff/1/?file=1818846#file1818846line12>
> >
> >     This is a change for the current behavior of the breadcrumbs in that currently
the last crumb is not clickable. Is this the intent? I think the current behavior is preferable,
but don't feel super strongly about it.
> >     
> >     Here's how I handled it in the original react prototype (which I remember feeling
was kind of hacky truth be told): https://github.com/jcohen/aurora-react/blob/master/src/components/Breadcrumbs.js

Changing current behavior wasn't intentional, I kinda just started from scratch and based
it on the github breadcrumb here: https://github.com/apache/aurora 

I don't have strong opinions about whether or not to change it, but one benefit is that on
more complex pages where we can modify the URL, it was nice to have a reset button right there
in the navigation.


> On Sept. 11, 2017, 4:18 p.m., Joshua Cohen wrote:
> > ui/src/main/js/components/Home.js
> > Line 4 (original), 3-5 (patched)
> > <https://reviews.apache.org/r/62135/diff/1/?file=1818847#file1818847line4>
> >
> >     Just curious, why the move away from the ES6 style?

Because of how Preact attaches names to custom components, the shallow rendering hack needs
an explicit name for the equality matching to work properly. You can still this with ES6 style,
but AFAICT you need the export default on a separate line?


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62135/#review185082
-----------------------------------------------------------


On Sept. 8, 2017, 11:40 p.m., David McLaughlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62135/
> -----------------------------------------------------------
> 
> (Updated Sept. 8, 2017, 11:40 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Kai Huang, and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Implementation of the home page in PreactJS. This was delayed heavily by the lack of
a shallow renderer (https://facebook.github.io/react/docs/shallow-renderer.html) for testing.
It was too painful to test without it (e.g. Links when fully rendered must be rendered within
a Router context), so I hand-rolled one based on some community attempts. The key motivation
is just to allow us to decouple markup from component logic, and also to avoid having to test
child components within component heirarchies. IMO the end result is very clean and easy to
read (and write!). 
> 
> If Preact gets a shallow renderer in the future, we can swap the one included here out.
> 
> 
> Diffs
> -----
> 
>   src/main/resources/scheduler/assets/images/aurora_logo_white.png PRE-CREATION 
>   ui/.eslintrc 355b6a8a5f5d25dd4c71dc546dc7d4acfffdd506 
>   ui/package.json f712518b27477bccb03f49c86eac3ee5f769fc88 
>   ui/src/main/js/client/scheduler-client.js PRE-CREATION 
>   ui/src/main/js/components/Breadcrumb.js PRE-CREATION 
>   ui/src/main/js/components/Home.js 91d60b387cf3b1fb268e73b7b50922a83898c31f 
>   ui/src/main/js/components/Icon.js PRE-CREATION 
>   ui/src/main/js/components/Loading.js PRE-CREATION 
>   ui/src/main/js/components/Navigation.js PRE-CREATION 
>   ui/src/main/js/components/RoleList.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/Breadcrumb-test.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/Home-test.js 2a80958d9303d1d3b9ae8f95013c66cb39f6bac3

>   ui/src/main/js/index.js 2f7467b41ac3373a90c5453a7534d384a585b464 
>   ui/src/main/js/pages/Home.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Home-test.js PRE-CREATION 
>   ui/src/main/js/utils/ShallowRender.js PRE-CREATION 
>   ui/src/main/js/utils/__tests__/ShallowRender-test.js PRE-CREATION 
>   ui/src/main/sass/app.scss PRE-CREATION 
>   ui/src/main/sass/components/_base.scss PRE-CREATION 
>   ui/src/main/sass/components/_breadcrumb.scss PRE-CREATION 
>   ui/src/main/sass/components/_home-page.scss PRE-CREATION 
>   ui/src/main/sass/components/_layout.scss PRE-CREATION 
>   ui/src/main/sass/components/_navigation.scss PRE-CREATION 
>   ui/src/main/sass/components/_tables.scss PRE-CREATION 
>   ui/src/main/sass/modules/_all.scss PRE-CREATION 
>   ui/src/main/sass/modules/_colors.scss PRE-CREATION 
>   ui/src/main/sass/modules/_typography.scss PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/62135/diff/1/
> 
> 
> Testing
> -------
> 
> ./gradlew ui:test
> 
> Running in vagrant (screenshots attached).
> 
> 
> File Attachments
> ----------------
> 
> preview
>   https://reviews.apache.org/media/uploaded/files/2017/09/08/011f99fb-a14e-4547-b90d-a5a1909c737c__Screen_Shot_2017-09-08_at_3.58.48_PM.png
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message