Import globals from internals

[Conduitry]

Hopefully fixes #2612 and #2947 and other similar types of potential issues. All (unless we missed some) globals used in code generated by Svelte now come from svelte/internals, so that we can avoid issues where other user variables shadow them.

This makes it an error to say @foo if foo is not actually exported from svelte/internal. All this affected currently was add_css, which has been rewritten slightly to involve a manual call to get_unique_name.

Thanks to @mrkishi for finishing this up!

[Rich-Harris]

Unfortunately this breaks tree-shaking — all the globals, used or not, end up in bundles. Haven't come up with a decent solution yet

[Conduitry]

Darn I had hoped Rollup would be able to cope with this. Would it be more treeshakable if we declared each variable separately, instead of destructuring all of them?

[Rich-Harris]

I had thought it would, but no :( It's possible that it thinks win.foo could be a getter with side-effects

[Rich-Harris]

(And even if Rollup could deal, there's still Other Bundlers to worry about)

[Conduitry]

Another idea from @mrkishi on how to handle this - have a single globals (or something) exported from svelte/internal that's either window or global (or self?) we could then have a special sigil that means 'import this globals reference, and assign a particular global in it to a deconflicted const, and use that'. My idea was to use something like @_foo to mean this - then we'd just have to have a rule that no regular svelte/internal exports could begin with an underscore.

[Conduitry]

Rebased on master and resolved conflicts - going to look into implementing the idea from the previous comment now.

[Conduitry]

There are still a couple of clunky things here, but the tests are passing for me locally, hopefully CI will agree.

[Conduitry]

@mrkishi Could you take a look at what I have here now? I'm wondering whether the change I had to make to the raf helper has something to do with the changes you made to the test init code - and I'm also wondering whether the test init code needs some updating or whether some of the changes there can be reverted, now that we're accessing globals differently.

I did undo your change to src/runtime/internal where each global that was used was imported from ./globals. That shouldn't actually be necessary if the bundler is doing its job. This could be what broke the thing with requestAnimationFrame.

[mrkishi]

Yeah, the thing with internal is that we need to add all the JSDOM window's properties to global in tests, since the tests all run in the plain node context. I assume removing the internal/globals imports worked because no tests rely on these, but like the requestAnimationFrame issue, we'd need to add them to global in helpers.js.

Should we maybe start running tests inside the JSDOM vm instead of globally?

[Conduitry]

Okay, I think we can worry about that in a separate PR. I've pushed an update to this one so that in helpers.js we initialize global.requestAnimationFrame = null; which appears to keep everything happy (presumably because we have set_raf that we use in tests).

[Conduitry]

I'm thinking it would make sense to only generate local aliases for globals that actually do conflict and need to be renamed. This would produce more normal-looking code more of the time. It might impact minifiability/gzipability, but I'm guessing that in general it really won't matter after gzip. If there aren't conflicts, this would produce the same code we are now without these changes.

I have changes locally to do this, and will tidy them up and push.

[mrkishi]

Other than this typo, this looks good to go, no?

[mrkishi]

Good thing I decided to comb one (last? please?) time. I don't see any more! (it said, again..)

I don't think it's worth it trying to improve the existing globals test because on top of defining the global we have to use relevant features to trigger the right codegen branch, and each time a new one is added we have to remember to update it.

While we should still drop the svelte$.compile() all the things, we could instead predefine all known globals to all tests' <script>, so when some code adds a new global, assuming there's a test for the feature, it'd also scream if it wasn't @_-fied.

Thoughts? That would go on a different PR though, of course.