Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass serverPath to init #11

Closed
domoritz opened this issue Jan 27, 2021 · 13 comments · Fixed by #12
Closed

Pass serverPath to init #11

domoritz opened this issue Jan 27, 2021 · 13 comments · Fixed by #12

Comments

@domoritz
Copy link
Contributor

I would like to publish a package to npm and therefore cannot predict the server path. I see that there is an init function that takes the path to the wasm file. Is there a way to generate a bundle that exposes this init function directly?

@Pauan
Copy link
Collaborator

Pauan commented Jan 30, 2021

Although you can technically use Rollup to create libraries (using the es output), it's not great at it.

Instead your library should simply publish ES6 modules, and it would be the end user's responsibility to use Rollup (or Webpack, or...)

Having said that, I would like to support this use case, but it is really tricky. So as a workaround you can simply use inlineWasm: true which will inline the Wasm into the JS file.

@domoritz
Copy link
Contributor Author

I've been using rollup to make bundles for libraries (https://www.github.com/vega/vega-lite) e.g. for browser use via a CDN. A use case is for example observablehq.

I have innlined wasm so far in my library (https://github.com/domoritz/arrow-wasm) but it would be nice to give people the option to provide a server path and reduce code size. I'd love if the generated init function took an optional path.

My hope is to generate different bundles that people can load and also provide es code for people who want to themselves bundle the library.

@Pauan
Copy link
Collaborator

Pauan commented Jan 30, 2021

I'd love if the generated init function took an optional path.

Yes, that's the end goal, though as I said it is tricky to implement.

domoritz added a commit to domoritz/rollup-plugin-rust that referenced this issue Jan 31, 2021
@domoritz
Copy link
Contributor Author

Would #12 work? I tried it locally and now this works

<html>
  <head>
    <meta content="text/html;charset=utf-8" http-equiv="Content-Type"/>
  </head>
  <body>
    <script src="./dist/hello.js"></script>
    <script>
      async function run() {
        // breaks
        // const module = await hello();

        // works
        const module = await hello('./dist/');

        // also works if we set the asset name
        // const module = await hello('./dist/hello.wasm');

        console.log(module.greet());
      }

      run();
    </script>
  </body>
</html>

rollup.config.js

import rust from "@wasm-tool/rollup-plugin-rust";

export default {
    input: "index.js",
    output: {
        file: "dist/hello.js",
        format: "umd",
        name: "hello"
    },
    plugins: [
        rust(),
    ],
};

lib.rs

use wasm_bindgen::prelude::*;

#[wasm_bindgen]
pub fn greet() -> String {
    "Hello, World!".into()
}

@domoritz
Copy link
Contributor Author

domoritz commented Jan 31, 2021

We could even do this

code: `
    import * as exports from ${import_path};
    ${prelude}

    async function init(input) {
        if (typeof input === 'string' && input.endsWith('/')) {
            input += ${import_wasm};
        }

        await exports.default(input ?? ${import_wasm});
        return exports;
    }

    export default Object.assign(init, exports);
`,

and then one could

await hello('./dist/');
console.log(hello.greet());

@domoritz
Copy link
Contributor Author

@Pauan what do you think?

@Pauan
Copy link
Collaborator

Pauan commented Feb 11, 2021

@domoritz Sorry for the delay. I don't think #12 will work, because there are three parts to the path:

  1. The filename (which is auto-generated by Rollup).
  2. The path relative from your library to the file.
  3. The path relative from the application to your library.

Your solution only allows for prepending the application path (3). But there are also legitimate reasons for the application to want to replace the path completely, so I think we need to support that as well.

@domoritz
Copy link
Contributor Author

domoritz commented Feb 11, 2021

Thanks for getting back to me. There is nothing to be sorry for.

My approach actually does support overwriting the path entirely. Only when the provided string ends with /, we prepend it to the filename. Otherwise, we replace it entirely.

@Pauan
Copy link
Collaborator

Pauan commented Feb 11, 2021

Ahh, right, that's an interesting approach. Let me think about it.

@domoritz
Copy link
Contributor Author

What do you think? I'm happy to make more changes if you see issues with the approach.

@Pauan
Copy link
Collaborator

Pauan commented Feb 21, 2021

After thinking about it, I don't like it, it seems too "magical". Instead, I think something like this would be better:

init({
    serverPath: (path) => "/foo/" + path
})

This has two big benefits:

  1. It allows us to add more options in the future.
  2. Since the function is passed the existing path, it can modify it however it wants: append, prepend, replace, etc.

@domoritz
Copy link
Contributor Author

Thanks for your feedback. I do like the idea of taking a function instead of a string to modify the path, though. However, I don't think we should use normal function arguments instead of an object.

I updated the code to support a function as you suggested.

@domoritz
Copy link
Contributor Author

domoritz commented Mar 1, 2021

How do you feel about my proposed solution?

@Pauan Pauan closed this as completed in #12 Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants