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

Use globalThis instead of window #28

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

pvcnt
Copy link

@pvcnt pvcnt commented Aug 29, 2024

Using window is not compatible with Web/Shared Workers (where window is undefined). Using globalThis is a better alternative, as it should work in all environments.

Using window is not compatible with Web/Shared Workers (where window is undefined). Using globalThis is a better alternative, as it should work in all environments.
@garronej
Copy link
Owner

Hello @PVNT,

Thank you for your PR! Before I proceed with merging, I want to ensure I fully understand how this change will benefit your use case. Beside there might be other areas in the code where similar updates are necessary.

I'm aware that support for web workers isn't ideal at the moment:
https://github.com/garronej/vite-envs-starter?tab=readme-ov-file#accessing-your-env-from-service-workers

While we could improve this by avoiding reliance on a global variable and instead replacing the variables throughout the code, this approach would require significantly more effort than the current implementation.

I'm having difficulty understanding how using globalThis would solve the issue with web workers, given that the script defining the global is injected into index.html, and a worker is initialized by a separate JavaScript script. Could you clarify this for me?

@pvcnt
Copy link
Author

pvcnt commented Aug 30, 2024

Hi @garronej, you are correct that it is not enough... I was trying to replace my own internal plugin by an open source plugin, and yours appeared to fit my needs, except for workers support.

The issue with this plugin is that there are two different syntaxes depending on whether we are in a worker (self.__VITE_ENVS) or not (import.meta.env). It makes impossible to write code that is agnostic of where it is running, or to reuse the same bit of code relying on an environment variable in both environments without some custom logic to switch from one to another.

In my current plugin, I ended up with the following to support workers (it's a bit convoluted, but it works well):

  • Using globalThis everywhere (and never self or window).
  • An injected script in index.html sets the default values, and imports a Javascript file that is templated (I use envsubst, but the approach with vite-envs.sh looks fine as well) and applies only the runtime overrides.
  • Workers are created like:
const worker = import.meta.env.DEV
    ? new SharedWorker(new URL("./instance", import.meta.url), { type: "module" })
    : new SharedWorker(new URL("./instance", import.meta.url), { type: "classic" });
  • In workers, the plugin injects the same snippet to set the default values, and another snippet (in production only) that uses importScripts to import the previous file that applies runtime overrides. This is why it is important to use classic workers in production, because it allows to use importScripts. In dev, we do not need those runtime overrides (they are already applied by the build system).

@garronej
Copy link
Owner

garronej commented Aug 30, 2024

@pvcnt You're absolutely right about everything.

I've realized that the WebWorker support offered doesn't work in dev mode. Since I haven't been using web workers in my recent projects, this issue slipped through the cracks—it's a classic case of not using your own tools.

Fixing this isn't trivial, but I can work on this. Alternatively, I'm open to reviewing a PR, though I should mention that the code for this module is quite convoluted.

If you're still interested in using it, would you submit a small PR to the starter repo? It would be helpful if you could set up a few examples involving web workers, along with any common gotchas.

On my end, I'll work on finding a solution based on your examples.

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 this pull request may close these issues.

2 participants