magento-research / peregrine

(Defunct) Moved to https://github.com/magento-research/pwa-studio
Open Software License 3.0
29 stars 1 forks source link

Added "MagentoRouter" component + tests #3

Closed DrewML closed 6 years ago

DrewML commented 6 years ago

Haven't wired it into the main library export yet (the "connector"), but unit tests are present. Please call out if I'm not testing any scenarios that you recognize.

This is primarily based off the HLD that Dan Mooney and I worked on.

There will be a wrapped <Link /> component later on that will allow us to skip the round trip to validate a route (in certain scenarios), but that's outside the scope of this PR.

Screenshot below is the setup I've been using in pwa-module-theme/theme to validate this. In a real usage, resolveUnknownRoute would be from an API module or something we've created, and the stubbed routes would be some generated artifact from the build that associates page components to a route def (next story). The Promise.resolve in getComponent would also be a fn using import() to lazily fetch the component def.

screenshot.

zetlen commented 6 years ago

In the absence of a Controller between routes and view components, what is responsible for kicking off all the other operations necessary to render a URL? The getComponent call is supposed to return a Promise for a Component, but should it simultaneously be launching requests for the relevant data? I don't think the components should define how to fetch their own data, because that's another waterfall: you can't get the data until the component has arrived and parsed.

So it seems like getComponent is a really big job...

DrewML commented 6 years ago

In the absence of a Controller between routes and view components, what is responsible for kicking off all the other operations necessary to render a URL? The getComponent call is supposed to return a Promise for a Component, but should it simultaneously be launching requests for the relevant data? I don't think the components should define how to fetch their own data, because that's another waterfall: you can't get the data until the component has arrived and parsed.

So it seems like getComponent is a really big job...

Will address the multiple questions/comments here

but should it simultaneously be launching requests for the relevant data

I don't believe so. If we're using Apollo on the client-side, each component in the tree dictates the data needed, and it's coalesced into a single request at the top. If we have the router aware of data fetching, we're either going to over-fetch or under-fetch.

If you wanted to change this, it's still compatible with this router implementation. Everything is just components - getComponent can easily choose a component that starts a data fetch for a known subset of data needed for the route, then kick off fetching the chunk needed at the same time.

I don't think the components should define how to fetch their own data, because that's another waterfall: you can't get the data until the component has arrived and parsed.

This is just a necessary evil for the type of abstraction we have chosen to use for data fetching. If each component gets to declare the data it needs, you'll never be able to fetch the data until post-parse.

DrewML commented 6 years ago

Looks like the changes on here are optional, so I'm going to merge so we can get the babel-eslint stuff in and get CI passing in Jimmy's PR. Happy to circle back later and iterate on the suggestions.