Remove all instances of optional chaining #489

martinamarien · 2021-08-25T19:22:42Z

Why are these changes introduced?
We have received some issues related to customers being unable to add items to their cart and checkout.
After looking into this a bit, it seems that optional chaining is likely the reason.

I am able to replicate this on versions of safari < 13.1

According to our shopify theme requirements, we require that our themes support the latest two versions of safari in Mac and iOS. Safari's current version is 14.1. Even if we did not need to support it, reverting this change is easy and allows for compatibility with older browser versions without the need of pollyfils, so it makes sense to make this change.

Fixes #266
Fixes #487

What approach did you take?
I removed all instances of ?. in Dawn. Now, we either check if the element is present first, and/or create a variable for the element.

this.header?.doSomething()

becomes

if (this.header) this.header.doSomething()

And

document.getElementById('section-header')?.doSomething()

Becomes

const header = getElementById('section-header');
if (header) header.doSomething();

Demo links

Store

Testing

Test every feature that was changed
Test on current versions and Safari 12.1
Can customers add to cart?

Checklist

assets/collection-filters-form.js

assets/pickup-availability.js

sections/main-product.liquid

chrisberthe · 2021-08-25T20:09:45Z

sections/main-product.liquid

@@ -466,14 +466,15 @@
        }
      )
      const activeMedia = this.querySelector(`[data-media-id="${this.openedBy.getAttribute("data-media-id")}"]`);
+      const activeMediaContent = activeMedia.querySelector('content');


Think there's something off here—I believe it should be updated to template and then we need to check for the .content property:

Suggested change

const activeMediaContent = activeMedia.querySelector('content');

const activeMediaContent = activeMedia.querySelector('template');

sections/main-product.liquid

ludoboludo · 2021-08-26T13:45:57Z

assets/cart-notification.js

@@ -38,7 +38,7 @@ class CartNotification extends HTMLElement {
          this.getSectionInnerHTML(parsedState.sections[section.id], section.selector);
      }));

-      this.header?.reveal();
+      if (this.header) this.header.reveal();
      this.open();


I don't get the cart notification to show up 🤔 on Safari 12.1

Really? I just tested it, and I'm able to get it to work with 12.1.

Screen.Recording.2021-08-26.at.4.44.37.PM.mov

If it worked for you then it should be good 👍 I will test again

Thanks Ludo

Confirmed some of those issues and created a follow up issue here: #510

Thank you 🙏🏻

assets/collection-filters-form.js

assets/global.js

assets/share.js

ludoboludo · 2021-08-26T14:43:11Z

sections/footer.liquid

      this.elements.input.value = event.currentTarget.dataset.value;
-      this.querySelector('form')?.submit();
+      if (form) form.submit();


Couldn't test this part as the country/region and language selectors wouldn't open

I was able to test and it seems to be working on 12.1 for me.

Created an issue for follow up: #510

ludoboludo · 2021-08-26T14:49:15Z

sections/featured-product.liquid

@@ -414,13 +414,14 @@
          }
        )
        const activeMedia = this.querySelector(`[data-media-id="${this.openedBy.getAttribute("data-media-id")}"]`);
+        const activeMediaContent = activeMedia.querySelector('content');


Similar comment here to what Chris mentioned on the main-product.liquid file. Though it's switching to the active media properly 🤔 the activeMediaContent returns a null value when I log it.

ActiveMediaContent is only being used for youtube videos it seems.
But I am actually unable to output any console messages when interacting with videos and 3D modals on the product page or in the modal.

@LucasLacerdaUX now that videos and modals are not being used to open the product modal, do we still need this code?

@LucasLacerdaUX now that videos and modals are not being used to open the product modal, do we still need this code?

They are still being used to open the modal, on mobile. I'm not sure if it is also true for featured product, tho. I'll check.

But why is this querySelector targeting a content element? In the original code, we are targeting the template and then retrieving its content (as in a property, not an actual element with the tag content). I haven't reviewed the full code so ignore my comment if it doesn't make any sense :P

I've tried both, and they didn't seem to make a difference, but I'll check specifically on mobile. It's possible I didn't test on an actual device. I forgot that on mobile, they are are all at as openers for the modal.

PaulNewton · 2021-08-27T09:17:32Z

@martinamarien For future reference, does optional chaining give any substantive performance benefits? aside from less code due to syntactic sugar.

If safari is updated it could be useful to know just how impactful refactoring optional chaining back in would be.

lakshanx · 2021-08-27T15:01:44Z

@PaulNewton I've had the same question a while back. And I've found some tests/articles online that say native optional chaining could be slightly performant than the other options in some cases. But then again, who'd run 10 million optional chaining operations in a real-world application?

It's a really good decision that you guys decided to remove optional chaining from Dawn. Safari backward compatibility is way more important than typing less code at the moment.

ludoboludo

Looking good. When I'm testing in browser stack though I get some issues with a few other things:

ResizeObserver isn't supported in versions of Safari prior to 13.4 (caniuse link).
I get errors when we try to call the method of custom element within another custom element 🤔 product modal and cart notification

But not related to optional chaining from what I can tell so can be tackled in another issue/PR.

Those errors I see them when testing in Browserstack on Mojave and Safari 12.1

ludoboludo

LGTM 👍

martinamarien · 2021-08-27T19:38:13Z

Hey @PaulNewton, like @lakshanx said, I don't think there's enough of a performance boost from using optional chaining, at least not in our case, to justify using a feature that breaks the purchase flow for these users. If this changes in the future, then we might re-evaluate its use in our themes. But for the time being, we've decided to remove it.

PaulNewton · 2021-08-28T07:37:27Z

⏳ Future note: specifically for liquid templates (not assets).
Before Optional Chaining gets reimplemented there's an argument first to consider that Optional Chaining is an anti-pattern in the liquid templates, of shopify themes. What it's doing is choosing programmers syntactic sugar over readable verbosity; particularly through the lenses of support troubleshooting and the merchant perspective.

What programmer has never missed a dot(.) ?
Even a seemingly simple ❔ question mark increases the complexity merchants need to successfully interpret a programmers single-line-of-code "elegance", and be able to synthesize solutions from these inline functional approaches versus a procedural approach that's following a familiar pattern of assigned element variables in the javascript that's similar to the more verbose liquid used in template files.

Add yet another symbol that merchants, or programmers, can miss in typing or reading... so now standalone lines as relatively simple as document.querySelector('form')?.submit(); instantaneously become social land mines:
document.querySelector('form').submit();
(for example if a setting turns the form off but not the javascript 💣 💥 !)

On the flip side of this False positive Optional chaining can silently fail which compounds common troubleshooting steps for templates with even simple typos(either in the javascript or the html).
document.querySelector('farm')?.submit(); <-- does not blow up is that good or bad? 😭

Given the above vagaries it could be argued further without a robust testing process as part of the dawn repo that Optional Chaining should only be allowed in build processes, if at all.

💭 In javascript assets this consideration could ignored as long as assets are consistent in using Optional Chaining over variable assignment and null checks, intermittent usage should probably be avoided and enforced with linting from theme check being updated with Optional Chaining rules.

* remove all instances of optional chaining * fix reviewer comments * fix formtting * fix youtube media on mobile viewports * fix spacing

remove all instances of optional chaining

6e2e208

chrisberthe reviewed Aug 25, 2021

View reviewed changes

ludoboludo self-requested a review August 26, 2021 13:36

ludoboludo reviewed Aug 26, 2021

View reviewed changes

martinamarien added 2 commits August 26, 2021 17:36

fix reviewer comments

4c847f3

fix formtting

ddc08a6

martinamarien requested review from chrisberthe and ludoboludo August 26, 2021 21:50

ludoboludo previously approved these changes Aug 27, 2021

View reviewed changes

fix youtube media on mobile viewports

bfcfee2

martinamarien dismissed ludoboludo’s stale review via bfcfee2 August 27, 2021 18:02

ludoboludo previously approved these changes Aug 27, 2021

View reviewed changes

chrisberthe previously approved these changes Aug 27, 2021

View reviewed changes

fix spacing

360e4c8

martinamarien dismissed stale reviews from chrisberthe and ludoboludo via 360e4c8 August 27, 2021 20:08

ludoboludo approved these changes Aug 27, 2021

View reviewed changes

chrisberthe approved these changes Aug 27, 2021

View reviewed changes

martinamarien merged commit 15e044c into main Aug 27, 2021

martinamarien deleted the revert-optional-chaining branch August 27, 2021 20:14

KaichenWang mentioned this pull request Feb 1, 2023

Fix quantity rules when variant is switched #2251

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove all instances of optional chaining #489

Remove all instances of optional chaining #489

martinamarien commented Aug 25, 2021 •

edited

Loading

chrisberthe Aug 25, 2021

ludoboludo Aug 26, 2021

martinamarien Aug 26, 2021 •

edited

Loading

ludoboludo Aug 26, 2021

martinamarien Aug 26, 2021

ludoboludo Aug 27, 2021

martinamarien Aug 27, 2021

ludoboludo Aug 26, 2021

martinamarien Aug 26, 2021

ludoboludo Aug 27, 2021

ludoboludo Aug 26, 2021

martinamarien Aug 26, 2021

LucasLacerdaUX Aug 27, 2021 •

edited

Loading

martinamarien Aug 27, 2021

PaulNewton commented Aug 27, 2021

lakshanx commented Aug 27, 2021

ludoboludo left a comment

ludoboludo left a comment

martinamarien commented Aug 27, 2021

PaulNewton commented Aug 28, 2021

	const activeMediaContent = activeMedia.querySelector('content');
	const activeMediaContent = activeMedia.querySelector('template');

Remove all instances of optional chaining #489

Remove all instances of optional chaining #489

Conversation

martinamarien commented Aug 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinamarien Aug 26, 2021 • edited Loading

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

Choose a reason for hiding this comment

LucasLacerdaUX Aug 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaulNewton commented Aug 27, 2021

lakshanx commented Aug 27, 2021

ludoboludo left a comment

Choose a reason for hiding this comment

ludoboludo left a comment

Choose a reason for hiding this comment

martinamarien commented Aug 27, 2021

PaulNewton commented Aug 28, 2021

martinamarien commented Aug 25, 2021 •

edited

Loading

martinamarien Aug 26, 2021 •

edited

Loading

LucasLacerdaUX Aug 27, 2021 •

edited

Loading