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

Webworker bundling issue prevents proper rendering in dev mode #79

Open
eacaps opened this issue Aug 11, 2023 · 7 comments
Open

Webworker bundling issue prevents proper rendering in dev mode #79

eacaps opened this issue Aug 11, 2023 · 7 comments

Comments

@eacaps
Copy link

eacaps commented Aug 11, 2023

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: #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
Copy link
Contributor

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
Copy link
Author

eacaps commented Aug 16, 2023

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
Copy link
Contributor

tonilastre commented Aug 16, 2023

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:

  • I would suggest adding a PR against the 1.0.0 because API changes a lot, especially around different orb views. We have 2 medium-sized chunks of work left to do for 1.0.0 to be released. By the way, to keep you in the loop, we are missing these two functionalities in 1.0.0:

    • Graph model reactivity for the fixes in the branch simulator-fixes - it contains several simulator performance fixes and an ability to make a node sticky (not moving) while other nodes move if physics is turned on. Currently graph model is not in sync with those changes and you as a user of the lib have no way to know in which state the simulator is
    • General model + style reactivity with the renderer FPS - we are missing various setters for node/edge data and style properties where you as a library user don't need to care about calling render, but the lib will take care of that internally by caring about the defined FPS rendering speed if possible.
    • Both of these features will change the API, so that's why we wait for those to be completed to release 1.0.0. We don't want to release the current 1.0.0 and then after a few weeks have a breaking change of 2.0.0. :)
  • As I talked before, the Orb will be a lib where you can implement your own simulator (currently the engine is d3) or even your own renderer. So to somehow prepare for it, I would maybe propose the following settings structure:

  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
Copy link
Author

eacaps commented Aug 17, 2023

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
Copy link

@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
Copy link
Author

eacaps commented Aug 25, 2023

@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
Copy link

@eacaps Thanks man 🙏

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

No branches or pull requests

3 participants