memgraph / orb

Graph visualization library
Apache License 2.0
364 stars 17 forks source link

Webworker bundling issue prevents proper rendering in dev mode #79

Open eacaps opened 1 year ago

eacaps commented 1 year ago

Hi! Awesome work you all are doing here on a solid graph library. I've stumbled upon a few issues integrating this into a Vue 3.0 project where we are using the memgraph backend. We're using vite to build our application and when running in dev mode it does not handle the webworker bundling correctly from WebWorkerSimulator:

        this.worker = new Worker(new URL(
        /* webpackChunkName: 'process.worker' */
        './process.worker', import.meta.url), { type: 'module' });

Unfortunately it fails in a way that is not caught by the SimulatorFactory, so it does not fall back to the MainThreadSimulator properly and the graph does not end up rendering properly. vite support for webworkers in dev mode is spotty at best, so i ended up just creating a by-pass so that SimullatorFactory would return a MainThreadSimulator in dev mode. I browsed around and found this related issue: https://github.com/memgraph/orb/issues/43 but that is running the vite preview command which works around the dev mode issues.

Like I said, I've coded up a hack for now, but looking through the issues and Pull requests I was wondering what the best approach would be to resolve this issue. I was considering adding an optional SimulatorFactory in the options to provide library consumers with the ability to handle the Simulation creation if they'd like and I can submit an external Pull request from a fork, but I also see there is potentially a lot changing in Release 1.0.0. Is this still being considered as a release or what are your thoughts on outside contributions?

tonilastre commented 1 year ago

Hey @eacaps, thanks for checking out the lib!

Regarding the worker, I agree, it definitely adds a bit of confusion upon configuration. So, in order for it to work, your framework (in this case Vue) should create a final build where the Orb worker is not bundled up together with the app code, but bundled as a separate script. For example, when integrating Orb to Angular app, you need to have this in the angular.json:

...
            "scripts": [
              "./node_modules/@memgraph/orb/dist/simulator/types/web-worker-simulator/simulator.worker"
            ]
...

What it will do is take the built worker script and bundle it along with the application. Then, when Orb wants to get a worker script, it will be there. Can you do the same?


Feature requests

Actually, you stumbled upon the vision of the Orb, which is to create such API where each of the underlying components can be changed. Orb is still in the early stage (thus the reason why 1.0.0 is not yet merged to the main) and what we want is to have all the Orb pipeline components (currently it is Graph (data), Style (integrated into graph), Renderer and Simulator) changeable. For now, Renderer is Canvas based, but we want to be able to provide support for WebGL support or even Canvas SVG support.

For the Factory, I am curious to see if the build configuration will do the trick. Feel free to propose the change via PR of how you would like to be able to handle SimulatorFactory - you can even create an issue with the proposal before coding it up and we + collaborators can discuss it there.

Thanks again for your interest. We should definitely add more items and missing pieces for version 1.0.0 to speed up the development and get to v1.0.0 quicker.

eacaps commented 1 year ago

Thank for the reply, I played around with it a bit, i think the equivalent in vite.config.ts is:

    optimizeDeps: {
      include: [
        "./node_modules/@memgraph/orb/dist/simulator/types/web-worker-simulator/simulator.worker",
      ],
    },

but i couldn't quite get it working right. Using web workers from external packages in vite in dev mode is apparently a pain, so i'll revisit this later. My proposed fix would look something like this:

(in src/views/default-view.ts)

    this._simulator = context.simulationFactory
      ? context.simulationFactory.getSimulator()
      : SimulatorFactory.getSimulator();

this would add an optional simulationFactory to IOrbViewContext, I wasn't sure if context or settings would be the appropriate place for it.

and then in our vue code we have this:

  const orb = new Orb<MyNode, MyEdge>(container);
  orb.setView((context) => {
    const simulationFactory = import.meta.env.DEV
      ? {
          getSimulator: () => {
            return new MainThreadSimulator();
          },
        }
      : undefined;
    return new OrbView({
      simulationFactory,
      ...context,
    });
  });

OrbView is just a local copy of default-view.ts with the above change. When I looked through the 1.0.0 PR I saw a lot of this stuff is changed there, so I was hesitant to open a pull request against main. Let me know what you think and I can open a PR against main or the 1.0.0 branch if you'd like. Everything runs fine when building and running preview with vite(npx vite build;npx vite preview) even without any orb specific modifications to the vite.config.ts so this is really just a dev mode only issue.

tonilastre commented 1 year ago

Aha, I get it now. I thought it is a general vite issue, I missed the dev mode part. I am just curious, what actually breaks, or which error is thrown and where? You said that it is not catched in the SimulatorFactory.


Regarding the change, the proposal makes sense. Few notes for easier integration and better future proof:

  instances: {
    simulator: ISimulator | () => ISimulator;
  }

Here in the future, we can add the renderer instance too. You, as a library user, can provide an instance of the simulator or even a callback that will create a simulator on its call. This way, you can easily separate types for IOrbViewSettingsInit vs IOrbViewSettings. The first one is used only on the init, while the second one is used to update settings anytime. I would propose having these instances in the ...Init type because it makes sense to handle the instances on View initialization. Getting into instance handling while orb view is active can yield a lot of sync issues which is definitely out of scope now. You can find these types in the orb-view.ts and orb-map-view.ts.

eacaps commented 1 year ago

When running in dev mode using vite, it looks for process.worker in the vite deps folder:

Request URL:
http://localhost:8080/node_modules/.vite/deps/process.worker?type=module&worker_file
Request Method:
GET
Status Code:
404 Not Found
Remote Address:
127.0.0.1:8080
Referrer Policy:
strict-origin-when-cross-origin

Like I mentioned I tried including various files using the optimizeDeps.include in vite.config.ts which pulls the files into the correct folder but it still wasn't quite working. Unfortunately, I still don't have a good idea of the best approach for fixing this.

As for the other changes, I will take a look at the 1.0.0 branch and create a PR based on our conversation.

lobo-tuerto commented 1 year ago

@eacaps Hey, I'm checking out Orb in my Vue 3 app and hitting the same issue as you.

It renders fine in preview mode, but that's not really helpful when trying to quickly iterate in dev.

Would you mind sharing your hack details to make it work in dev while the issue at large gets fixed?

eacaps commented 1 year ago

@lobo-tuerto I have created an example repo: https://github.com/eacaps/orb-hack. See the PR for more details. @tonilastre you can checkout the initial commit on that PR to see the error in the developer console.

lobo-tuerto commented 1 year ago

@eacaps Thanks man :pray: