In order to cleanup the message of <script> at the top of paymentRequest.xhtml and duplicated in our mochitests, we should switch to using ES modules to import dependencies. :jonco, any reason we shouldn't start using them for code targeting ~Fx63 on Nightly by default?

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #0) No reason, please go ahead. Let me know if there are any issues.

Comment on attachment 8967611 [details] Bug 1446179 - Use ES Modules for the unprivileged Payment Request dialog code. https://reviewboard.mozilla.org/r/236326/#review242112 ::: toolkit/components/payments/res/PaymentsStore.js (Diff revision 2) > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > -"use strict"; Btw. strict mode is implied by ES modules and eslint complained that this was now useless ::: toolkit/components/payments/res/components/payment-details-item.js:30 (Diff revision 2) > > constructor() { > super(); > this._label = document.createElement("span"); > this._label.classList.add("label"); > - this._currencyAmount = document.createElement("currency-amount"); > + this._currencyAmount = new CurrencyAmount(); The changes from `createElement` to constructing an instance directly wasn't necessary to be done at this point but it helps a lot with linting and getting imports correct because the class can be easily determined without the customElements registry lookup (which can't be done statically). ::: toolkit/components/payments/res/paymentRequest.xhtml:136 (Diff revision 2) > - height="400" > + height="400"></iframe> > - src="debugging.html"></iframe> This is the reason for the `toggleDebuggingConsole` change… The problem was that debugging.js tries to synchronously access the payment-dialog's requestStore but since type="module" is async, it may not exist yet. By delaying the console load until later we avoid this problem. This also means that we won't needlessly load debugging.html for users who never look at it i.e. the common case.

Comment on attachment 8967612 [details] Bug 1446179 - Update payments tests to use ES modules. https://reviewboard.mozilla.org/r/236328/#review242118 ::: toolkit/components/payments/.eslintrc.js:11 (Diff revision 2) > + "test/mochitest/test_*.html", > ], > parserOptions: { > sourceType: "module", Ideally this wouldn't be needed but eslint-plugin-html isn't the smartest: https://github.com/BenoitZugmeyer/eslint-plugin-html/issues/91 ::: toolkit/components/payments/test/mochitest/mochitest.ini:7 (Diff revision 2) > - ../../res/paymentRequest.css > + ../../res/** > - ../../res/paymentRequest.js > - ../../res/paymentRequest.xhtml `support-files` is unusual… there is a major difference in behaviour between using a glob and not… with a glob, the resulting test files use a directory structure mirroring the source directory but without globbing (how it was), the files are all copied to the matching directory of the ini file i.e. …/mochitest/ in this case. This meant that our testign directory structure didn't match what we actually shipped and that caused problems for the module import paths as they wouldn't match in tests. This fixes them to use the source directory structure for consistency. ::: toolkit/components/payments/test/mochitest/mochitest.ini:22 (Diff revision 2) > [test_payment_details_item.html] > [test_payment_method_picker.html] > [test_rich_select.html] > [test_shipping_option_picker.html] > [test_ObservedPropertiesMixin.html] > +[test_PaymentsStore.html] This test had to be moved from xpcshell since there isn't a way to use ES modules from xpcshell-test yet: bug 1453816.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aff4e1110fc93ff38c577e00513389f96d43cfd1

Comment on attachment 8967611 [details] Bug 1446179 - Use ES Modules for the unprivileged Payment Request dialog code. https://reviewboard.mozilla.org/r/236326/#review242344 ::: toolkit/components/payments/res/mixins/PaymentStateSubscriberMixin.js:59 (Diff revision 2) > * > * Attaches `requestStore` to the element to give access to the store. > * @param {class} superClass The class to extend > * @returns {class} > */ > -function PaymentStateSubscriberMixin(superClass) { > +export function PaymentStateSubscriberMixin(superClass) { As the file is named after this function, I think it makes sense to make this export the default for this file.

Comment on attachment 8967611 [details] Bug 1446179 - Use ES Modules for the unprivileged Payment Request dialog code. https://reviewboard.mozilla.org/r/236326/#review242344 > As the file is named after this function, I think it makes sense to make this export the default for this file. yeah, definitely, somehow I misunderstood documentation and thought that a `default` could only be used if there was one export but that doesn't make sense.

Comment on attachment 8967612 [details] Bug 1446179 - Update payments tests to use ES modules. https://reviewboard.mozilla.org/r/236328/#review242792 ::: toolkit/components/payments/test/mochitest/test_order_details.html:67 (Diff revision 3) > > add_task(async function test_initial_state() { > setup(); > is(orderDetails.mainItemsList.childElementCount, 0, "main items list is initially empty"); > is(orderDetails.footerItemsList.childElementCount, 0, "footer items list is initially empty"); > - is(orderDetails.totalAmountElem.value, 0, "total amount is 0"); > + is(orderDetails.totalAmountElem.value, "0", "total amount is 0"); Out of curiousity, was this causing a test failure? ::: toolkit/components/payments/test/mochitest/test_rich_select.html:15 (Diff revision 3) > <script type="application/javascript" src="/tests/SimpleTest/SpawnTask.js"></script> > <script type="application/javascript" src="/tests/SimpleTest/EventUtils.js"></script> > <script src="payments_common.js"></script> > - <script src="custom-elements.min.js"></script> > - <script src="ObservedPropertiesMixin.js"></script> > - <script src="rich-select.js"></script> > + <script src="../../res/vendor/custom-elements.min.js"></script> > + > + <link rel="stylesheet" type="text/css" href="../../res/components/rich-select.css"/> Continuing with the module pattern, it may make more sense for the initialization of a custom element to append the stylesheet for itself to the DOM.

Comment on attachment 8967646 [details] Bug 1446179 - Support ES modules in browser_parsable_script.js. https://reviewboard.mozilla.org/r/236382/#review242796

Comment on attachment 8967612 [details] Bug 1446179 - Update payments tests to use ES modules. https://reviewboard.mozilla.org/r/236328/#review242792 > Out of curiousity, was this causing a test failure? Yes it was… the value comes from the attribute which is a string. I'm not sure why this bug caused the failure though. > Continuing with the module pattern, it may make more sense for the initialization of a custom element to append the stylesheet for itself to the DOM. Yeah, that could be an enhancement but I'd rather switch to Shadow DOM when it's ready so the module can have a scoped stylesheet.

Pushed by mozilla@noorenberghe.ca: https://hg.mozilla.org/integration/autoland/rev/64f5c8475f3b Use ES Modules for the unprivileged Payment Request dialog code. r=jaws https://hg.mozilla.org/integration/autoland/rev/4057d947afae Update payments tests to use ES modules. r=jaws https://hg.mozilla.org/integration/autoland/rev/5fe5bf607ad6 Support ES modules in browser_parsable_script.js. r=jaws

https://hg.mozilla.org/mozilla-central/rev/64f5c8475f3b https://hg.mozilla.org/mozilla-central/rev/4057d947afae https://hg.mozilla.org/mozilla-central/rev/5fe5bf607ad6