Add useful errors on bad use of Promise instead of instance in new MODULARIZE

[kripken]

It used to be possible to do

Module().onRuntimeInitialized = ...

but with new MODULARIZE (#10697) Module() returns a Promise
instead. The Promise of course allows a property to be placed on it, but
nothing would happen, so it's potentially annoying for users. With
this change, when ASSERTIONS are on we will throw an error
with an explanation, something like

You are setting onRuntimeInitialized on the Promise object, instead
of the instance. Use .then() to get called back with the instance, see
the MODULARIZE docs in src/settings.js

Likewise, it used to be possible to do

var instance = Module();
// .. later ..
instance.exportedThing()

but again, the "instance" is a Promise now, and the exports aren't
there. This will crash, but ".. is not a function" is not that helpful.
With this change it will give an explanation of what's wrong.

cc @curiousdannii @lourd

[lourd]

I think this would be more readable using a template literal:

  return `if (!Object.getOwnPropertyDescriptor(${promise}, ${property})) {
  Object.defineProperty(${promise}, ${property}, { configurable: true, get: function() { abort('You are getting ${property} on the Promise object, instead of the instance. Use .then() to get called back with the instance, see the MODULARIZE docs in src/settings.js') } });
  Object.defineProperty(${promise}, ${property}, { configurable: true, set: function() { abort('You are setting ${property} on the Promise object, instead of the instance. Use .then() to get called back with the instance, see the MODULARIZE docs in src/settings.js') } });
}`;

If you wanted to get even more DRY:

  const warningMsg = `${property} on the Promise object, instead of the instance. Use .then() to get called back with the instance, see the MODULARIZE docs in src/settings.js`;
  return `if (!Object.getOwnPropertyDescriptor(${promise}, ${property})) {
  Object.defineProperty(${promise}, ${property}, { configurable: true, get: function() { abort('You are getting ${warningMsg}') } });
  Object.defineProperty(${promise}, ${property}, { configurable: true, set: function() { abort('You are setting ${warningMsg}') } });
}`;

[kripken]

Good idea, thanks! Yeah, we should use more modern JS, now that we require Node 12.9.* at least anyhow.

[sbc100]

Hmm.. we still need to re-land the node version requirement: https://github.com/emscripten-core/emscripten/pull/9345/files

[kripken]

Oh, I forgot about that...

@sbc100 do we need to do anything more than update all node versions in the emsdk manifest file to 12.9.1?

[sbc100]

No I think we just need to update the re-land that old PR

[sbc100]

This makes me think we should have some kind of documentation on our use of JS both in the compiler output and in the compiler internals.

I think it would be very useful to be able to point to when doing JS code reviews like.

It sounds like using template literals is fine, both in the generated code and in the compiler code?

[kripken]

Not sure about the generated code. When we emit code for legacy browsers we may want to avoid that. Node 4 is 5 years old, but people may want to target even older things...

But yeah, I'll look into documenting this stuff, separately from this PR, good idea.

[curiousdannii]

IMO you shouldn't be worried about supporting platforms past their EOL if there are features that would make a real improvement to Emscripten's output. IE 11 is the only one still being supported that doesn't support template literals. If it was up to me I'd say that if anyone needs IE 11 support they can put Emscripten's output through Babel, but I'm not the one supporting Emscripten, so you choose what you want :)

[sbc100]

Update the range of supported targets, and possibly raising the min requirments is certainly something we can and should do. There are users who are targeting very old and embedded environment though so expect some push back.

In any case that is a larger discussion and IMHO outside the scope of this change.

[kripken]

I agree it's out of scope. I do like the idea though, and since we've improved our npm integration, I wonder if we can just emit new JS by default and run babel at the end optionally? We've discussed similar things before but it seems more practical now.

[kripken]

I'd like to land this as it may help issues being filed like WebAssembly/wabt#1421 (comment) - any objections?