Sapper: Dynamic import in client.js breaks app in legacy browsers with v0.28

[rodoch]

Describe the bug
It can be advantageous to use dynamic imports in client.js, e.g. for browser feature detection and polyfilling:

import * as sapper from '@sapper/app';

const config = {
    target: document.querySelector('#sapper')
};

if (!Intl.PluralRules) {
    (async () => {
        await import('@formatjs/intl-getcanonicallocales/polyfill');
        sapper.start(config);
    })();
} else {
    sapper.start(config);
}

Prior to Sapper v0.28.0 this worked without issue. In v0.28.0+ using dynamic imports in client.js causes the app to break, i.e. an uncaught error causes JS to cease executing. Looking at the generated output it seems that somehow using a dynamic import means __inject_styles is called prior to the line __inject_styles = __inject_styles.default. The error message is given below.

Logs
The following error is displayed in the browser console when using a legacy browser:

Uncaught in promise TypeError: __inject_styles is not a function

To Reproduce
I have included a minimal repro here: https://github.com/rodoch/sapper-client-dynamic-import-repro

This is the default Sapper template with the addition of @rollup/plugin-json and @formatjs/intl-getcanonicallocales/polyfill. The only changes to the app code have been made in client.js.

To reproduce:

1. Clone the repo and install dependencies.
2. npm run build && node __sapper__/build.
3. View in Browserstack with a legacy browser such as <= Chrome 54.

Expected behavior
Including a dynamic import in client.js should not break the app. It is possible to prevent this issue by statically importing the files as below, however this means you are not doing feature detection and you are shipping polyfill code to everybody including browsers that don't need it.

import * as sapper from '@sapper/app';
import '@formatjs/intl-getcanonicallocales/polyfill';

sapper.start({
    target: document.querySelector('#sapper')
});

Information about your Sapper Installation:

  System:
    OS: Windows 10 10.0.18363
    CPU: (8) x64 Intel(R) Core(TM) i7-4770HQ CPU @ 2.20GHz
    Memory: 8.12 GB / 15.88 GB
  Binaries:
    Node: 12.13.0 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.4 - C:\Program Files (x86)\Yarn\bin\yarn.CMD
    npm: 6.13.1 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Chrome: 86.0.4240.75
    Edge: Spartan (44.18362.449.0)
    Internet Explorer: 11.0.18362.1
  npmPackages:
    rollup: ^2.3.4 => 2.29.0
    sapper: ^0.28.0 => 0.28.10
    svelte: ^3.17.3 => 3.29.0

• Browser as described above.

• Local, Azure deployment.

• Dynamic application

Severity
Not a total blocker, but unnecessarily increases bundle size for all users.

If, for some reason, this is not something which can be supported going forward I recommend it be noted as a breaking change in the Sapper migration docs.

[benmccann]

One of the tests does a dynamic import: https://github.com/sveltejs/sapper/blob/master/test/apps/css/src/routes/bar.svelte

I guess the question is what's different about your case than the test case

[rodoch]

The test case has a dynamic import inside onMount inside of a component. I'm experiencing no issues in that situation. My issue appears specific to dynamic imports called in client.js (i.e. in the client.js file itself).

[benmccann]

The root cause of this seems to be an issue in Shimport: https://github.com/Rich-Harris/shimport/issues/36

I've raised a PR to revert the original fix since it would break source maps

[rodoch]

The plot thickens! Thanks for this.

[Rich-Harris]

(FYI that Shimport bug is now fixed)

[Conduitry]

:+1: Yup, this should be good now with Shimport 2.0.5, even if you're using the upcoming version of Sapper that reverts the improper fix in #1595.