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

fix(init): exclude the no-window lint rule for new projects #2351

Closed
wants to merge 3 commits into from

Conversation

rojvv
Copy link
Contributor

@rojvv rojvv commented Mar 8, 2024

No description provided.

Copy link
Contributor

@deer deer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for this? I think new projects should be abiding by this rule. I'm not sure why we would exclude it.

@rojvv
Copy link
Contributor Author

rojvv commented Mar 9, 2024

So do you prefer self.addEventListener("click", /* ... */); instead of window.addEventListener("click", /* ... */); or what?

See https://discord.com/channels/684898665143206084/991511118524715139/1205582216244240524.

@deer
Copy link
Contributor

deer commented Mar 9, 2024

As per #2351 (comment)

The migration is straightfoward - replace all usages of window with globalThis or self.

Personally I've used globalThis, but apparently self is also a valid fix.

@rojvv
Copy link
Contributor Author

rojvv commented Mar 9, 2024

Even MDN examples are using window.. What do you say on this, @marvinhagemeister? Should Fresh be the one enforcing users to use globalThis. for client-side code?

@marvinhagemeister
Copy link
Collaborator

I don't have strong opinions on this. Sometimes I'm using window, sometimes globalThis. That said window doesn't work everywhere and we'll be trying to deprecate it at some point in Deno. So globalThis is definitely more future proof.

@rojvv
Copy link
Contributor Author

rojvv commented Mar 12, 2024

Which browsers don’t support window?

@marvinhagemeister
Copy link
Collaborator

Which browsers don’t support window?

I don't understand what you're hinting at.

@rojvv
Copy link
Contributor Author

rojvv commented Mar 12, 2024

You told me that window doesn’t work everywhere. By everywhere, did you mean across different browsers or you meant worker environments?

@marvinhagemeister
Copy link
Collaborator

Right, I meant in future Deno versions. Having the window global in Deno has always been a thorn as it breaks so many "is server" checks in the ecosystem. So there is an incentive to get rid of it. With Fresh running code both on the server and the client, you might need to know which context you're in in the future to be able to use window.

@rojvv
Copy link
Contributor Author

rojvv commented Mar 12, 2024

I’m talking about browsers.

@marvinhagemeister
Copy link
Collaborator

I’m talking about browsers.

How does the lint rule detect whether the code you're authoring runs in the browser or in Deno?

@rojvv
Copy link
Contributor Author

rojvv commented Mar 12, 2024

That doesn’t mean it should be discouraged for browser-side use.

@deer
Copy link
Contributor

deer commented Mar 14, 2024

Hi @roj1512, let me take a step back and try to clarify the entire situation. I've just now realized that my previous link was incorrect -- I wanted to link to this: denoland/deno#22057 (comment). The deno team (of which I'm not a part of, so I don't really know why) has decided to remove support for window in deno 2.0. There's a helpful linting error introduced now in order to assist people with cleaning up their code before it breaks when upgrading to 2.0.

Fresh (obviously) uses deno, and, more importantly, runs all client side code on the (deno based) server prior to sending it to the browser. (There is an enhancement to support not running islands on the server -- see #975.) Given that the islands run on the server, they can't have window in 2.0, or they'll fail.

So this brings me back to the original issue here: I don't think it's a good idea to disable this linting rule, since it helps people migrate to 2.0 The paths forward seem to be:

  1. complain to the core deno team that support for window shouldn't be dropped
  2. push forward the client only islands issue, so that islands using window don't get run on the fresh server

@rojvv rojvv closed this Mar 14, 2024
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.

3 participants