URL Resolver #64
URL Resolver #64
Conversation
- Add initial version URL resolver that passes spec tests
- Add additional validation to URL resolver - Update indicator of Service resolver per spec change - Re-factor Abstract Resolver and Factory to support deprecated indicators - Update tests
| @@ -53,11 +53,11 @@ public function resolve($definition) | |||
| $renderData = []; | |||
|
|
|||
| if ($definition->has('provide')) { | |||
| foreach ($definition->get('provide') as $index => $definition) { | |||
There was a problem hiding this comment.
Is this part of the PR functionality or just a little naming cleanup you did along the way?
There was a problem hiding this comment.
Yeah, two birds one PR to close #62 as well.
|
|
||
| return $uri->getHost() === self::FAKE_BASE_HOST | ||
| ? str_replace(self::FAKE_BASE_URL, '', $returnUrl) | ||
| : $returnUrl; |
There was a problem hiding this comment.
In the spec, these values are supposed to be usable either as strings via coercion, or as objects with URL part properties: https://github.com/magento-research/pwa-studio/tree/develop/packages/upward-spec#urlresolver-example (last line of the section).
Looks like this returns a string. I don't see a test for the property lookup behavior. If it's hard to implement, we could revise the spec to lose that requirement (though I'd prefer it stay in). What do you think?
There was a problem hiding this comment.
Not intentional, just an oversight. That seems simple enough, though the return type in upward-js appears to be a node URL class, or an object with only a subset of those properties. Should I follow that pattern in this implementation?
|
Refreshed cloud instance with this change to UPWARD and latest copy of Venia. Will run regression tests, and will likely just merge on pass with @dpatil-magento approval. We'll be running a full in depth QA once the resolver is implemented into Venia in scope of #1166. https://master-7rqtwti-xjmpcdjpe6kzm.us-4.magentosite.cloud/ |
There was a problem hiding this comment.
👍- - - - - - - - - - 👍👍- - - - 👍👍👍👍- - - -👍 - - - - - 👍
👍- - - - - - - - -👍- - - 👍- - - - - -👍- - - - - - - 👍👍- 👍👍
👍- - - - - - - - -👍- - - - - - - - - - - 👍- - - - - - - 👍- -👍- -👍
👍- - - - - - - - -👍- 👍👍- - - - - -👍- - - - - - - 👍- - - - - -👍
👍👍👍👍- - - - 👍👍- - - - - - -👍- - - - - - - 👍- - - - - -👍
No description provided.