Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat(upward): add UrlResolver and tests. Closes #1057 #1058

Merged
merged 7 commits into from
Apr 17, 2019

Conversation

zetlen
Copy link
Contributor

@zetlen zetlen commented Mar 25, 2019

Description

Add an eighth Resolver to UPWARD, the UrlResolver.

  • Add the resolver to upward-js, along with unit tests.
  • Add tests for the UrlResolver to upward-spec, the cross-platform test suite.
  • Add documentation to the README for UPWARD.
  • Modify venia-upward.yml to use the UrlResolver instead of TempateResolvers to join URLs.

Related Issue

Closes #1057.
Related to #1045 and other issues resolving URLs for ServiceResolvers or ProxyResolvers..

Motivation and Context

See #1057; URLs should be a first class concept in UPWARD.

Verification

Experiment with URL resolution based on the documentation. Try different configurations in Venia.

How Has This Been Tested?

Full test coverage and integration test suite coverage.
Tried all cases in Venia where UPWARD resolves URLs. This resolver simplifies the Venia UPWARD definition and makes it more reliable.

Proposed Labels for Change Type/Package

FEATURE in upward-js
ENHANCEMENT in venia-concept

Checklist:

  • I have read the CONTRIBUTING document.
  • I have linked an issue to this PR.
  • I have indicated the change type and relevant package(s).
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All CI checks are green (linting, build/deploy, etc).
  • At least one core contributor has approved this PR.

@vercel
Copy link

vercel bot commented Mar 25, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

@jcalcaben
Copy link
Contributor

jcalcaben commented Mar 29, 2019

The docs look good to me. Unassigning myself so others can pick this up and review the implementation.

@jcalcaben jcalcaben removed their assignment Mar 29, 2019
@tjwiebell tjwiebell self-assigned this Apr 3, 2019
Copy link
Contributor

@tjwiebell tjwiebell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, some minor changes should make this a little easier to understand. If we've got sign off from the team that this is a priority, I can jump on the upward-php implementation, so start brushing up on your PHP so you can review that for me.

@tjwiebell
Copy link
Contributor

@zetlen - Ran a quick validation of this strategy, and we can move the venia-concept change to a separate PR that waits for upward-php, and merge the rest now. This would also allow us to publish an alpha release of upward-spec so I can more easily test and write the upward-php implementation.

What do you think of that approach?

@coveralls
Copy link

coveralls commented Apr 16, 2019

Coverage Status

Coverage increased (+0.4%) to 76.822% when pulling 666dfa6 on zetlen/upward-add-url-resolver into 6129afe on develop.

@tjwiebell tjwiebell removed their assignment Apr 16, 2019
@zetlen
Copy link
Contributor Author

zetlen commented Apr 17, 2019

@magento-research/core-pwa-devs Any other concerns before we merge?

@zetlen zetlen mentioned this pull request Apr 26, 2019
2 tasks
@sirugh sirugh added the version: Minor This changeset includes functionality added in a backwards compatible manner. label May 24, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
version: Minor This changeset includes functionality added in a backwards compatible manner.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

URLs are hard to accurately build ing UPWARD
7 participants