System.js build: Fix leaking variables, improve resource resolution logic

[mofux]

• Wraps the polyfills and loading logic inside a self-executing function to avoid leaking variables to the window object. Fixes System.js build is leaking variables to window object #1786
• First looks for a script with data-stencil-namespace or data-resources-url to resolve the resources url, instead of always using the last script in the DOM. Fixes System.js build can resolve wrong resources url when included with defer attribute #1787, fixes Component lazy loader detects wrong base URL #1456

[manucorporat]

do we need call() here?
can't it be })()?

[mofux]

That's what I've tried initially, but it turned out that the function scope was not bound to window when using })(). This did break the embedded ES6Promise polyfill unfortunately. Using .call(window) therefore explicitly sets the function scope to the window object, making the ES6Promise polyfill work again.

[mofux]

I've updated the script resolution logic to first look for a script with the data-stencil-namespace property set to the namespace property configured in the stencil.config.ts. This is the most reliable way to tell the loader which script it should use to resolve the resourcesUrl from.

[manucorporat]

@mofux I think it's called [data-namespace], not [data-stencil-namespace], no?
would you mind creating a different PR with the fix for the defer though?

Btw, would you mind adding the explanation of call(window) as comment?

Awesome job figuring all this out!

[mofux]

@manucorporat Sure, I'll split out the defer fix to a different PR.

The [data-stencil-namespace] attribute was introduced by myself and shouldn't be referred to anywhere outside the system.js loader itself... Do you want me to change it to [data-namespace] instead? I think data-stencil-namespace is a better name because it is scoped to stencil.

[mofux]

Making two PRs out of this at the same time seems... complicated 🤔 The changes of one PR would conflict with the other. I could make one PR (defer) based on the other (var leak), but then the second PR would be identical to this one 😅

I guess it's best to remove the var leak stuff from this PR, then get this one merged, and then add another PR for the var leak based on the updated master branch?

I'd like this PR to become the one that fixes the defer stuff, because the comments and referenced issues mostly talk about that topic.

[manucorporat]

So far, I can't merge the current PR since it would break existing applications. Also what's the point of adding defer? just curious

[mofux]

So far, I can't merge the current PR since it would break existing applications

The changes should be fully backwards-compatible now. There is no need to specify the data-stencil-namespace attribute with the script tag, but if you do, chances that the correct script is picked are much higher 😅 If not specified it will simply continue with the old lookup logic instead.

Also what's the point of adding defer? just curious

You can use the defer or async attributes to delay execution of the script. IMO this is very useful when combining it with prerendered components because it can speed up the time to the first meaningful draw (the JS parsing can run after the page has already painted).

[manucorporat]

@mofux I know... but modern browser (all but IE11) will use type="module" which will not even run all this code. I guess why try to optimize IE11.

ok, now you fixed it: 0bc27ec

:)

[mofux]

but modern browser (all but IE11) will use type="module" which will not even run all this code. I guess why try to optimize IE11.

Well, we had to opt out of the type="module" script because of two reasons:

• It doesn't contain all the polyfills. We had a problem with Firefox ESR 60 that has no native support for customElements, so the module script failed on it.
• The CMS of my customer doesn't support the ES6 modules (it does some magic around the scripts for it's live editing features)

IMO it is a wrong assumption to assume that the nomodule script is only used in IE11. It works perfectly fine in all other browsers as well, and there may be many reasons to prefer the nomodule script over the module script.

[mofux]

@manucorporat I just noticed commit 3a44def. Will this solve the leaking variables problems, or is this addressing a different problem?

[XianHain]

The changes should be fully backwards-compatible now. There is no need to specify the data-stencil-namespace attribute with the script tag, but if you do, chances that the correct script is picked are much higher 😅 If not specified it will simply continue with the old lookup logic instead.

Is there a way to check to see if the namespace is a part of the script URL? I'm seeing an issue where this code is intermittently changing the URL of a third-party script that exists on the page outside of the Stencil Web Component and changing the filename to corePath.

For example, it's changing https://foo.com/analytics.js to https://foo.com/p-80f7b62d.system.js, breaking both the web component and the third-party script.

I've opened issue #1827 with more details.