getsentry / spotlight

Your Universal Debug Toolbar
https://spotlightjs.com
Other
353 stars 12 forks source link

Spotlight Code is bundled twice in Astro Dev mode #68

Open Lms24 opened 9 months ago

Lms24 commented 9 months ago

let's see if we can keep this closed

Lms24 commented 9 months ago

nope...

Lms24 commented 9 months ago

OK So I think I figured out the cause:

We bundle two versions of Spotlight code in different places and dispatch to the "wrong" event target

I think we have two solutions:

  1. Simply initalize and bootstrap Spotlight within the plugin bundle and don't inject it into the page bundle. This would be easiest but has two disadvantages:
    • we can't initialize Spotlight if the toolbar is not activated by users
    • we/users can't pass options to the Spotlight.init call in the plugin because currently, you can't pass any data/arguments to the plugin initialization function.
      1. Put the event target onto the global object. This makes sure what there's only one instance and we can listen and dispatch to it from both instances.

I think, given the limitations of 1, I'm gonna go with 2.

Oh btw I have no idea about why this didn't cause problems within the monorepo. My best guess is that Vite and pnpm resolve and bundle dependencies differently than when a package is added as a dependency. This gives me trust issues in our monorepo 😅

Lms24 commented 9 months ago

So, this has all to do with how and where we import spotlight from.

  1. In the overlay plugin, we import * as Spotlight from @spotlightjs/core because importing from @spotlightjs/astro (i.e.its own package) throws errors
  2. In the page snippet we inject, we import * as Spotlight from @spotlightjs/astro because importing from @spotlightjs/core throws an error (at least in pnpm which requres all deps to be directly specified in package.json).

Not sure if we can get around this easily...

matthewp commented 9 months ago

I don't think double-bundling is something that should happen. I could see this as perhaps being a race-condition where both modules are loaded in parallel resulting in them being double-loaded.

What I'd try first is listing these packages in Vite's optimizedDeps config here: https://vitejs.dev/config/dep-optimization-options.html#optimizedeps-include

Doing this will cause Vite to pre-bundle them, in which case it should see that there is common dependencies between them and prevent the duplication. If that doesn't work, let me know and I'll think of something else.

How you would do that in an Astro integration is with updateConfig

Lms24 commented 9 months ago

@matthewp thanks for looking into this!

I gave it a quick shot and optimizedDeps seems to fix it on first glance. However, in pnpm-based projects, vite will complain about not being able to resolve @spotlightjs/core. I think that's because the package is a dependency of @spotlightjs/astro and therefore not explicitly declared in users' package.jsons.
If I only include "@spotlightjs/astro" in the optimizedDeps array, the original problem still persists (which makes sense afaict)

Any further ideas?

Fwiw, I pushed a workaround in #121 which also resolves the problem but it doesn't fix the double bundling.

Lms24 commented 9 months ago

Gonna remove from the 1.0 milestone because it's unlikely that we fix this properly before 1.0. Also edited the title to be more precise.

BYK commented 1 month ago

@Lms24 how can I tell if this is still an issue or not after the general Vite plugin approach?

Lms24 commented 1 month ago

how can I tell if this is still an issue or not after the general Vite plugin approach?

I guess we'd need to remove putting the spotlight event target (see eventTarget.ts) onto the global object and only rely on the event target in the module scope. Then try to open/close spotlight in the Astro dev toolbar.

However, given the age of this issue, if things work fine for now (with the global even target), I suggest we close this and reopen/open if we get reports. wdyt?