Rename wasmImpots to envImports and move actual wasmImports to top level scope

[toyobayashi]

As #20035 (comment) proposed, move imports definition outside the createWasm function for allowing user specify custom namespace in JS library.

I'm not very familiar with emscripten codebase and can only make such a simple change, not sure what will be affected. it looks like all the variables used in the imports are in the same scope so maybe it's not a problem? Feel free to close this if it really breaks something.

[sbc100]

While I kind of like the simplicity of this solution (as proposed in #20035 (comment)).. I'm also wondering if there is a better way to can officially support the two level namespace.

Adding a new global called imports and making part of the ABI seems a little risky, since it would be hard to then change in the future.

I also wonder if we can come up with a better name than just imports here. Its a shame wasmImports is already taken. Perhaps we should rename the existing wasmImports to envImports so we can use wasmImports for this new global?

[sbc100]

If we are making the imports object global, then perhaps we just remove the third argument from this function completely?

[toyobayashi]

found it used elsewhere

emscripten/src/library_async.js

Lines 541 to 550 in 0a6f54b

	_load_secondary_module: async function() {
	// Mark the module as loading for the wasm module (so it doesn't try to load it again).
	wasmExports['load_secondary_module_status'].value = 1;
	var imports = {'primary': wasmExports};
	// Replace '.wasm' suffix with '.deferred.wasm'.
	var deferred = wasmBinaryFile.slice(0, -5) + '.deferred.wasm';
	await new Promise((resolve) => {
	instantiateAsync(null, deferred, imports, resolve);
	});
	},

[toyobayashi]

Indeed, I personally think that the __namespace decorator seems to be a better way, but when I wanted to try to do this, I found that changing the code is not trivial, and sadly there is nothing I can do, so I thought of this simple way.

The name "imports" was chosen because I see that MINIMAL_RUNTIME did it. Now have been renamed according to your opinion.

[toyobayashi]

Can such changes be merged?

[sbc100]

I believe this was fixed in a separate PR: #20231. Sorry I forgot we had two different PRs for the same thing

[toyobayashi]

I believe this was fixed in a separate

@sbc100 I think this PR and #20231 are not talking about the same thing. How does #20231 allow me specify custom namespace for JavaScript library symbols? The most fundamental purpose of this PR is moving the original importsObject outside createWasm function.

[toyobayashi]

It’s been a month since #20035 was opened and you may have forgotten the details. Initially, I hoped to add the __namespace modifier to provide this feature, but it was too difficult for me to make code change for the emscripten codebase and you may have no time to do such things, so I proposed a simple solution (#20035 (comment)), which is to move info outside the createWasm function. After your suggestion, I renamed wasmImport to envImport and rename the info to wasmImport. Changes in #20231 seems not related to import object and still not provide a way that can solve #20035.

[sbc100]

Ah yes, sorry I see that #20231 was related to same variable but not trying to solve the same issue.

[toyobayashi]

Branch updated. Please take a look if you have time. If there is a better way to solve #20035 in a short time, I don't mind closing this PR and waiting for official support.

[sbc100]

I would love to find a way to fix #20035 in a nicer way. I did take a stab at it a few days ago but I ran into some roadblock. In the long term I do think its something we do want to support, I'm just not clear what the best path forward is at this moment. This change is looks little invasive, which is making me think twice about it.

[toyobayashi]

Thank you for your nice work. I'm looking forward to good news. Once this feature is supported, then I can advance the header change landing on Node.js side.

[toyobayashi]

any progress/update on this?