> 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 > >
