Remove use of eval in non-legacy rollup builds

[seanlail]

Description

As @maximedupre reported in #1622:

This file contains new Function (injected in the browser), which is a form of eval, which causes a CSP error, unless adding unsafe-eval

Adding unsafe-eval to CSP would then defeat the purpose of using CSP.

It was using new Function to do feature detection for imports, but it looks like that is now supported by all non-legacy browsers anyway.

This PR removes the try/catch when building a non-legacy rollup bundle, thereby removing the use of shimport.
This will allow users to remove "unsafe-eval" from any CSP configuration.

I realise Sapper is in maintenance but I feel like this is a blocking issue for production usage.

Closes #1622
Relates to #343

Test

I came across this issue originally because I added CSP + blocked unsafe-eval and noticed that reactivity did not work.
So I've added a test that does just that. Binds a text input, types in a value and then checks that the updated value is shown.
This test now passes, before the change it failed.

Questions

• Should I add the nonce_attr in?
• Should I just use the webpack line? (i.e. </script><script${nonce_attr} src="${main}" defer>

[benmccann]

what was this try / catch trying to detect? I feel like it was to see what browser you were using and can't simply be removed

[seanlail]

Looks like it was trying to detect "import" support to add the module script, otherwise it added "shimport".

[Conduitry]

If there's a way to detect ESM/import/import() support without a try-catch, then I'd be all for switching to that. Otherwise, as I indicated below, I don't think we can just do this without it being a breaking change. (And it'd be a breaking change that I don't think could be easily worked around for people that need to support these browsers. They'd be stuck on the old version.)

[seanlail]

@Conduitry Agreed. I'll have a look 👍

[Conduitry]

I'd be wary about making this without it being a breaking change. Are there, say, any old version of Chromium on old web TVs that support async/await but do not support ESM? I would imagine so, and I know there are people who use Sapper for these environments.

[seanlail]

I'd be wary about making this without it being a breaking change. Are there, say, any old version of Chromium on old web TVs that support async/await but do not support ESM? I would imagine so, and I know there are people who use Sapper for these environments.

@Conduitry Good point, this would be a breaking change. If those people have not targetted legacy then shimport could be saving them at the moment.

Could we rewrite that try/catch then to be CSP safe? 🤔

[Kapsonfire-DE]

'noModule' in HTMLScriptElement.prototype
can be used without try catch to detect module functionality

[Conduitry]

Does that safely check for dynamic import support as well? There was a while where Firefox supported import/export by not import(). We still need to bring in Shimport if the browser supports import ... from ... but not import(...).

[seanlail]

Unfortunately I think 'new Function' is the way to test dynamic.

Could we solve this another way?

Default to using shimport and then expose a flag for people to skip/disable it?

It's like the opposite of legacy though.

[Conduitry]

If there's not a reasonable way to test for dynamic imports, then I think at this point in Sapper's life, the answer for how to do this in a CSP-compatible way is to use a fork of Sapper. There's not a good way to expose options like this without there being an overbearing number of CLI options.

I would encourage you to follow along with SvelteKit and, when we get to a point where we begin to think about differential legacy builds there, to advocate for whatever features you think are necessary to keep CSP happy.

[Kapsonfire-DE]

But whats wrong with try catch?

[Kapsonfire-DE]

try { import('').catch(ex => {}); } catch(ex) { console.log('dynamic export not supported'); }
I mean this works perfectly without any console output

[Conduitry]

I haven't checked, but won't import('') be a parsing error on some browsers? If it won't even parse, this won't work, and none of the code will run. The eval was to get around stuff not parsing.

[Kapsonfire-DE]

either dynamic import exists, then it returns a promise and its error is catched with .catch(ex), or import doesnt exists and its throws a default error catched by try-catch... if import('') really does a parsing error, then you can use any valid string here
import('notExistant.js')
still same result

[Kapsonfire-DE]

oh you are right - giving syntax error on IE

[seanlail]

@Kapsonfire-DE @Conduitry Thanks for digging in to this.

I have added a comment to 2 SvelteKit issues and will follow the progress.

I'm going to fork Sapper I think to get around the issue for now, as like you said there won't be many changes to Sapper anyway and hopefully we can move to Kit soon.

Thanks for looking at this anyway 👍