Refactor create-account route to use appShell to get provided requisite properties #1430

sirugh · 2019-07-10T15:19:33Z

Description

The create-account page (and probably other pages) was broken. Upward used index when serving /create-account. index uses generic-shell.mst, which uses default-initial-contents.st, which references assets.svg.logo. This means that anything using default-initial-contents.mst must also include the asset manifest.

Related Issue

Closes #1429.

Verification Steps

Run this PR locally and go to <url>/create-account. It should show the create account page.

Screenshots / Screen Captures (if appropriate)

Proposed Labels for Change Type/Package

major (e.g x.0.0 - a breaking change)
minor (e.g 0.x.0 - a backwards compatible addition)
patch (e.g 0.0.x - a bug fix)

Checklist:

I have updated the documentation accordingly, if necessary.
I have added tests to cover my changes, if necessary.

…nders

vercel · 2019-07-10T15:19:41Z

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

Latest deployment for this branch: https://venia-git-fix-create-account-link.magento-research1.now.sh

sirugh · 2019-07-10T15:35:20Z

@dpatil-magento not sure why this is failing to build -- I ran the failing command locally and it worked. :(

dpatil-magento · 2019-07-10T15:43:15Z

@sirugh Looks like the backing instance is down. Checking this now - https://venia-cicd-lrov2hi-mfwmkrjfqvbjk.us-4.magentosite.cloud/

PWAStudioBot · 2019-07-10T16:21:13Z

	Messages
📖	We are currently working on automating the PR metadata checks. Until that time, you may see failures related to labels/description/linked issues/etc even if you have fixed the problem. Failures will persist until the next push (assuming they are fixed).

Generated by 🚫 dangerJS against 8e157a9

zetlen

Excellent thank you!

dpatil-magento · 2019-07-15T21:33:25Z

@sirugh After creating account on https://venia-qbl9lrsc0.now.sh/create-account, I see js error.

Also, whats the expected behavior if Customer is already logged in and then access create-account direct URL?
I tried this on Luma .... and it redirects customer to /customer/account/ and does not show /customer/account/create/ (create account page)

cc @soumya-ashok @awilcoxa as its related to UX.

sirugh · 2019-07-16T13:58:29Z

I'll look into the bug @dpatil-magento.

Also it looks like we have two paths to /create-account, one where the user is on the sign-in page and clicks the create account button, and the other where the user navigates directly to the /create-account url. Now that I realize this, I'm not sure what the intended UX was. Do we even want a /create-account page in venia or do we just want users to create via the sidebar/login page? Adding needs-ux label.

dpatil-magento · 2019-07-16T14:40:48Z

@sirugh the /create-account page already exists when guest user place an order and then click on Create Account button.

soumya-ashok · 2019-07-16T14:46:04Z

@dpatil-magento and @sirugh

First off, I see this as being such an edge case where someone takes the trouble to save the create account URL and then tries to visit it via the URL especially on shopping sites where they really don't want to create an account in the first place unless there's a compelling reason.

However, if we do want to cover this scenario, we could have the following cases for the sign-in/create account experience

If the user has created an account and is signed in, the footer within the main nav. shows that, like in this mockup - https://magento.invisionapp.com/share/KMT00QQH79C
In this case, unless the user signs out of their account, they would not see the sign-in/create account page. If they do try to visit the create account page via URL while signed in, we should probably allow them to create an account with a different email, but if they try to enter the same email as the one they're currently using, then just show the error we already have for that use-case.

If the user has an account but is signed out and visits the create account page, then they should be able to create an account with an email other than the one they're using, and if they try to use the existing email, display the appropriate error.

Does that answer the question?

sirugh · 2019-07-16T15:17:26Z

@soumya-ashok as @dpatil-magento showed there are two routes to create an account.

Guest makes purchase and clicks Create Account and is redirected to /create-account. This opens the form as the whole page.
User clicks menu hamburger and Sign In and then Create Account. This opens the form on the side menu.

I don't think we're talking about a situation where a user is navigating directly to /create-account via an external link or anything.

soumya-ashok · 2019-07-16T16:23:23Z

@sirugh I see. One of the comments above said "navigates directly to the create account URL", so I thought that meant they were going to it outside of the path provided.
Is there a specific reason why the two routes @dpatil-magento specified are different? I had assumed that clicking create account would lead to the form in the side menu.

…rce object schema

sirugh · 2019-07-16T17:30:49Z

packages/venia-concept/upward.yml

-# An object that returns true if the NODE_ENV environment variable is set
-# to 'development'. Used as a test in other configurations in this file.
-# Returns false otherwise.
-isDevelopment:


Unused properties, AFAICT.

sirugh · 2019-07-16T17:33:10Z

packages/venia-concept/upward.yml

    - matches: request.url.pathname
      pattern: '^/create-account'
-      use: index
+      use: appShell


Instead of having two routes, index vs appShell or hardcodedAppShell vs urlResolvedAppShell as @zetlen and I discussed, I went with this method in which I re-used the single appShell. The benefit being that I did not have to rewrite much of the foundation that appShell already provides.

sirugh · 2019-07-16T17:34:27Z

packages/venia-concept/src/util/makeUrl.js

+const htmlDataset = document.querySelector('html').dataset;
+const { imageOptimizingOrigin } = htmlDataset;
+// Protect against potential falsy values for `mediaBackend`.
+const mediaBackend = htmlDataset.mediaBackend || 'https://backend.test/media/';


This fixes a bug identified where mediaBackend was an empty string which destructures as "" instead of being falsy.

TBH I'm not sure why we have a default value here anyways -- seems like it just gets rewritten in the makeUrl code.

Yeah, there shouldn't be a default here.

Ok -- rather than remove the default, @zetlen and I discussed and it is out of scope. That being said we should probably rewrite makeUrl or at least fix this to not need a fallback (ie. console.error and THEN use a default.

I just added a warning if it's falsy to at least indicate a problem.

sirugh · 2019-07-16T17:37:39Z

packages/venia-concept/upward.yml

@@ -33,11 +33,10 @@ response:
    - matches: request.url.pathname
      pattern: '^/(graphql|rest)(/|$)'
      use: proxy
-    # Requests to create an account are handled by the top-level 'index'
-    # object, which returns a generic app shell
+    # Requests to create an account are handled by the 'appShell' object.
    - matches: request.url.pathname


Also, it's not clear to me why lines 37-39 are required. I would have assumed we could just resolve similar to how we resolve /search.html, but if I remove these lines I get failures like unable to load "<url>/dist/create-account"

Actually, you've uncovered an optimization opportunity in the resource definition. Matches are evaluated top to bottom, so we want the hardcoded matches to be above the matches that check urlResolver. When we know ahead of time that the hardcoded URL will not be resolved with urlResolver, we want to avoid evaluating urlResolver, since it's actually placing a graphQL query. There's gotta be a simpler way to do that in the future.

For now, to prevent merge conflicts, let's just move all the match clauses against literal URL paths to the top of the conditional stack, and all the matches against urlResolver under them. That helps us avoid an additional request.

dpatil-magento · 2019-07-17T03:21:20Z

@sirugh QA Pass, can be merged once review is approved.

zetlen

Just reorder some conditionals to avoid unnecessary urlResolver calls and go forward with it. I realize that this means testing against the same URL patterns in multiple places. We can fix that later. Thanks for all your help.

zetlen · 2019-07-17T15:00:15Z

packages/venia-concept/src/util/makeUrl.js

+const htmlDataset = document.querySelector('html').dataset;
+const { imageOptimizingOrigin } = htmlDataset;
+// Protect against potential falsy values for `mediaBackend`.
+const mediaBackend = htmlDataset.mediaBackend || 'https://backend.test/media/';


Yeah, there shouldn't be a default here.

zetlen · 2019-07-17T15:08:22Z

packages/venia-concept/upward.yml

@@ -33,11 +33,10 @@ response:
    - matches: request.url.pathname
      pattern: '^/(graphql|rest)(/|$)'
      use: proxy
-    # Requests to create an account are handled by the top-level 'index'
-    # object, which returns a generic app shell
+    # Requests to create an account are handled by the 'appShell' object.
    - matches: request.url.pathname


Actually, you've uncovered an optimization opportunity in the resource definition. Matches are evaluated top to bottom, so we want the hardcoded matches to be above the matches that check urlResolver. When we know ahead of time that the hardcoded URL will not be resolved with urlResolver, we want to avoid evaluating urlResolver, since it's actually placing a graphQL query. There's gotta be a simpler way to do that in the future.

For now, to prevent merge conflicts, let's just move all the match clauses against literal URL paths to the top of the conditional stack, and all the matches against urlResolver under them. That helps us avoid an additional request.

zetlen

This is good now.

provide assetManifest to index object for use in generic app shell re…

db741de

…nders

sirugh added the version: Patch This changeset includes backwards compatible bug fixes. label Jul 10, 2019

PWAStudioBot added the pkg:venia-concept label Jul 10, 2019

awilcoxa assigned zetlen Jul 15, 2019

Merge branch 'develop' into fix/create-account-link

7ca642d

vercel bot deployed to staging July 15, 2019 19:45 View deployment

zetlen previously approved these changes Jul 15, 2019

View reviewed changes

zetlen assigned dpatil-magento and unassigned zetlen Jul 15, 2019

Merge branch 'develop' into fix/create-account-link

71434f7

vercel bot deployed to staging July 15, 2019 21:24 View deployment

sirugh added the needs-ux Collaboration and review from UX team is required label Jul 16, 2019

sirugh self-assigned this Jul 16, 2019

sirugh added 2 commits July 16, 2019 12:28

Fix default fallback for empty string case

e01dc44

Refactor Create account route to use appShell to reuse existing resou…

1bbf584

…rce object schema

sirugh dismissed zetlen’s stale review via 1bbf584 July 16, 2019 17:30

vercel bot deployed to staging July 16, 2019 17:30 View deployment

sirugh commented Jul 16, 2019

View reviewed changes

add word Page

36a97df

vercel bot deployed to staging July 16, 2019 17:35 View deployment

sirugh assigned zetlen and unassigned sirugh Jul 16, 2019

sirugh changed the title ~~provide assetManifest to index object for use in generic app shell renders~~ Refactor create-account route to use appShell to get provided requisite properties Jul 16, 2019

sirugh commented Jul 16, 2019

View reviewed changes

Merge branch 'develop' into fix/create-account-link

cd545ec

vercel bot deployed to staging July 17, 2019 02:53 View deployment

zetlen suggested changes Jul 17, 2019

View reviewed changes

Merge branch 'develop' into fix/create-account-link

8c3694f

vercel bot deployed to staging July 17, 2019 15:41 View deployment

vercel bot deployed to staging July 17, 2019 15:45 View deployment

Reorder upward resource resolver and default mediaBackend to origin

7c83336

sirugh force-pushed the fix/create-account-link branch from a18b5f6 to 7c83336 Compare July 17, 2019 15:48

vercel bot deployed to staging July 17, 2019 15:48 View deployment

Add a warning if dataset attribute is falsy

8e157a9

vercel bot deployed to staging July 17, 2019 15:58 View deployment

zetlen approved these changes Jul 17, 2019

View reviewed changes

zetlen removed their assignment Jul 17, 2019

dpatil-magento merged commit 98eb578 into develop Jul 17, 2019

sirugh deleted the fix/create-account-link branch July 25, 2019 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor create-account route to use appShell to get provided requisite properties #1430

Refactor create-account route to use appShell to get provided requisite properties #1430

sirugh commented Jul 10, 2019

vercel bot commented Jul 10, 2019

sirugh commented Jul 10, 2019

dpatil-magento commented Jul 10, 2019

PWAStudioBot commented Jul 10, 2019 •

edited

Loading

zetlen left a comment

dpatil-magento commented Jul 15, 2019 •

edited

Loading

sirugh commented Jul 16, 2019

dpatil-magento commented Jul 16, 2019

soumya-ashok commented Jul 16, 2019

sirugh commented Jul 16, 2019

soumya-ashok commented Jul 16, 2019

sirugh Jul 16, 2019

sirugh Jul 16, 2019

sirugh Jul 16, 2019

zetlen Jul 17, 2019

sirugh Jul 17, 2019

sirugh Jul 17, 2019

sirugh Jul 16, 2019

zetlen Jul 17, 2019

sirugh Jul 17, 2019

dpatil-magento commented Jul 17, 2019

zetlen left a comment

zetlen Jul 17, 2019

zetlen Jul 17, 2019

zetlen left a comment

Refactor create-account route to use appShell to get provided requisite properties #1430

Refactor create-account route to use appShell to get provided requisite properties #1430

Conversation

sirugh commented Jul 10, 2019

Description

Related Issue

Verification Steps

Screenshots / Screen Captures (if appropriate)

Proposed Labels for Change Type/Package

Checklist:

vercel bot commented Jul 10, 2019

sirugh commented Jul 10, 2019

dpatil-magento commented Jul 10, 2019

PWAStudioBot commented Jul 10, 2019 • edited Loading

zetlen left a comment

Choose a reason for hiding this comment

dpatil-magento commented Jul 15, 2019 • edited Loading

sirugh commented Jul 16, 2019

dpatil-magento commented Jul 16, 2019

soumya-ashok commented Jul 16, 2019

sirugh commented Jul 16, 2019

soumya-ashok commented Jul 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpatil-magento commented Jul 17, 2019

zetlen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zetlen left a comment

Choose a reason for hiding this comment

PWAStudioBot commented Jul 10, 2019 •

edited

Loading

dpatil-magento commented Jul 15, 2019 •

edited

Loading