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

Spotlight Code is bundled twice in Astro Dev mode #68

Open
Tracked by #17
Lms24 opened this issue Nov 21, 2023 · 9 comments · Fixed by #104
Open
Tracked by #17

Spotlight Code is bundled twice in Astro Dev mode #68

Lms24 opened this issue Nov 21, 2023 · 9 comments · Fixed by #104
Assignees
Labels
Type: Bug-confirmed Something isn't working
Milestone

Comments

@Lms24
Copy link
Member

Lms24 commented Nov 21, 2023

No description provided.

@Lms24 Lms24 changed the title Dev overlay button doesn't open spotlight outside of Monorepo Astro Dev overlay button doesn't open spotlight outside of Monorepo Nov 21, 2023
@Lms24 Lms24 added the Type: Bug-confirmed Something isn't working label Nov 21, 2023
@Lms24 Lms24 self-assigned this Nov 21, 2023
@Lms24
Copy link
Member Author

Lms24 commented Nov 22, 2023

let's see if we can keep this closed

@Lms24 Lms24 added this to the 1.0 milestone Nov 22, 2023
@Lms24
Copy link
Member Author

Lms24 commented Nov 22, 2023

nope...

@Lms24 Lms24 reopened this Nov 22, 2023
@Lms24
Copy link
Member Author

Lms24 commented Nov 22, 2023

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

  • One time, the spotlight code is injected into the page bundle. This is fine and works just like our SDK in Astro. This version bootstraps itself, creates the shadow root, mounts the debugger window and waits for someone to open the app. For example, its eventTarget is listening for an "open" event.
  • The other time, the spotlight code is added to the Astro toolbar plugin bundle. Inside the init function of our Astro toolbar plugin file, we call Spotlight.open(). This call dispatches an "open" event to spotlight's eventTarget. However, the event target that this is dispatched on, is the one from the toolbar plugin bundle. While it should actually trigger the other 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
Copy link
Member Author

Lms24 commented Nov 22, 2023

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

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

Lms24 commented Nov 23, 2023

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

Lms24 commented Nov 28, 2023

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.

@Lms24 Lms24 changed the title Astro Dev overlay button doesn't open spotlight outside of Monorepo Spotlight Code is bundled twice in Astro Dev mode Nov 28, 2023
@Lms24 Lms24 modified the milestones: 1.0, 2.0 Nov 28, 2023
@Lms24 Lms24 modified the milestones: 2.0, 3.0 Jun 13, 2024
@BYK
Copy link
Member

BYK commented Jul 25, 2024

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

@Lms24
Copy link
Member Author

Lms24 commented Jul 29, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug-confirmed Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants