Supports dynamic imports in tests

[julienw]

Fixes #1204

[julienw]

This is untested. @mstange does it work for you on your current work ?

[mstange]

Thank you!! I'll try it out in a sec.

[codecov]

Codecov Report

Merging #1211 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1211   +/-   ##
=======================================
  Coverage   75.82%   75.82%           
=======================================
  Files         144      144           
  Lines        9321     9321           
  Branches     2297     2297           
=======================================
  Hits         7068     7068           
  Misses       2012     2012           
  Partials      241      241

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87eb4c7...082f9e1. Read the comment docs.

[gregtatum]

If we want to prefer static import rather than dynamic, then we should probably add comments linking to the issue to add this feature: webpack/webpack#6615

I'm not sure in this use-case what's appropriate.

[mstange]

Doesn't look like it helps unfortunately :(

[mstange]

I've found out a few things.

• If I don't have "syntax-dynamic-import" in the root plugins section (not in the env.test.plugins section!), then Jest gives me a different error:

      Jest encountered an unexpected token
      This usually means that you are trying to import a file which Jest cannot parse, e.g. it's not plain JavaScript.
      By default, if Jest sees a Babel config, it will use that to transform your files, ignoring "node_modules".
      Here's what you can do:
       • To have some of your "node_modules" files transformed, you can specify a custom "transformIgnorePatterns" in your config.
       • If you need a custom transformation specify a "transform" option in your config.
       • If you simply want to mock your non-JS modules (e.g. binary assets) you can stub them out with the "moduleNameMapper" config option.
      You'll find more details and examples of these config options in the docs:
      https://jestjs.io/docs/en/configuration.html

• If I replace the async import with a sync import, it encounters another error inside the imported module:

$ env NODE_ENV=test jest src/test/unit/symbol-store.test.js -i
 FAIL  src/test/unit/symbol-store.test.js
  ● Test suite failed to run

    /Users/mstange/code/perf.html/node_modules/gecko-profiler-demangle/gecko_profiler_demangle.js:2
    import * as wasm from './gecko_profiler_demangle_bg';
           ^

    SyntaxError: Unexpected token *

      22 |   // The name of the function that this address belongs to.
      23 |   name: string,
    > 24 |   // The offset (in bytes) between the start of the function and the address.
      25 |   functionOffset: number,
      26 | |};
      27 |

      at ScriptTransformer._transformAndBuildScript (node_modules/jest-runtime/build/script_transformer.js:316:17)
      at Object.<anonymous> (src/profile-logic/symbol-store.js:24:30)

(Do I need to pre-babelize gecko_profiler_demangle.js?)

[mstange]

    import * as wasm from './gecko_profiler_demangle_bg';
           ^

    SyntaxError: Unexpected token *

It looks like I'm not the first person to run into this, see rustwasm/wasm-bindgen#233 .

The "gecko_profiler_demangle" npm package has been created for the target environment "browser", not for the target environment "nodejs". As a result, it's not expected to work in node. And there is no good solution to target both yet. For example, here's how this problem was worked around in the wasm_cmark_parse module: https://github.com/FreeMasen/wasm_cmark_parse/blob/master/pkg/wasm_cmark_parse_bg.js

Julien, do you have an idea for how I could make the demangling code to only run in the browser and not under node?

[julienw]

From what I see, all that the syntax plugin does is accepting dynamic imports, I don't see any transformation done with this plugin.
I also see that webpack runs wasm-based tests using jest, so this should be doable.

Webpack's tests use wast-loader with this configuration. But I don't think this is necessary for us: wast-loader allows loading text-form of wasm files, which isn't our need. Otherwise Webpack defaults to webassembly/experimental for wasm files (after https://medium.com/webpack/webpack-4-released-today-6cdb994702d4).

Another supposition is that Jest supports dynamic import since jestjs/jest#5883... which is in Jest 23. Turns out I just merged the PR #1212 that upgrades Jest, so maybe after a rebase your code will work :)

To fully answer your question:

• I don't think the "target environment" is really checked by any tool.
• we can mock modules with Jest. Here is how you would mock a node module for all tests or in one test only. So this is a possible path. But it seems that we'd need to synchronously importing the wasm module and that can have user-facing issues (though maybe our module is small enough that we can do it ?).

And I do have a question for you: how does it work at runtime ? Does webpack "just work" with the dynamic import ?

[julienw]

I also found https://github.com/airbnb/babel-plugin-dynamic-import-node, but I think this is primarily for projects that don't use webpack.

[julienw]

Can you point me to your code that uses a dynamic import at the moment ? It would be easier to try to fix the problem :) Is it #1203 ?

[mstange]

Yes, it's #1203.

[mstange]

I've rebased #1203 onto current master and it didn't fix the problem.

[julienw]

@mstange, I looked closer from your code and read a bit more on jest documentation.

1. for jest, we'll actually need "dynamic-import-node" instead of "syntax-dynamic-import". This will replace import by require and (I hope!) make it work properly in node.
2. then, gecko_profiler_demangle.js actually uses import directly which isn't supported by node. To overcome just you can tell jest to actually run babel on some node_modules by customizing the transformIgnorePatterns configuration option, there's an example here: https://jestjs.io/docs/en/tutorial-react-native.html#transformignorepatterns-customization .
3. then (and that's where I am right now), the problem is that in node we can't require a wasm file. Node has the standard WebASsembly API that we need to use. That's what webpack is doing behind the scene when you instruct it to "import" a wasm chunk
4. ... so maybe that's exactly what you should do in gecko-profiler-demangle and ship the result of running webpack. Or write it manually, as you wish. Bottom line is that import XXX from './bla.wasm' doesn't seem to be standard. Instead you should write WebAssembly.instantiateStreaming(fetch(BLA)) which should be compatible with both the web and node, I think. And because this is asynchronous, we wouldn't need dynamic imports anymore ;)

[mstange]

Instead you should write WebAssembly.instantiateStreaming(fetch(BLA)) which should be compatible with both the web and node, I think.

I see. I can do that... I just need to get the right URL to fetch (does node support fetch?), I guess I'll use Webpack's file-loader with import to get me that URL?

So then the module require will be synchronous, but the functions that I export will need to resolve asynchronously because fetch is asynchronous.

[mstange]

Yeah, I'm not having much luck with getting the URL in the two contexts. In the webpack+browser context, I can use import wasm_path from 'file-loader!./gecko_profiler_demangle_bg.wasmpleaseignoremewebpack';. (With the .wasm extension I wasn't able to make webpack ignore the file contents and just give me a URL.)
But this doesn't work in the jest / node context.

[mstange]

I've added a commit onto #1203 that demonstrates what I tried.

[julienw]

So then the module require will be synchronous, but the functions that I export will need to resolve asynchronously because fetch is asynchronous.

Or you can use the sync API, maybe that's not that bad for this module...

Yeah, I'm not having much luck with getting the URL in the two contexts.

can't you use "./module" for the URL ?

But I now realize that means we'll need to copy the file manually... maybe it's just easier to mock the lib...

Another option to pack your wasm lib is https://github.com/rustwasm/wasm-pack. Did you look at it yet ?

[mstange]

There is no sync "fetch" for the browser... or maybe I'm misunderstanding what you're saying?

can't you use "./module" for the URL ?

In the node context: probably. In the browser + webpack context: not sure - webpack will need to know that this file is one that it needs to put into dist/ and add to the service worker cache.

[julienw]

closing it now that Markus will do it in his other patch