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

Modernize window.crypto access constants #4169

Merged
merged 12 commits into from
Apr 22, 2024
Merged

Modernize window.crypto access constants #4169

merged 12 commits into from
Apr 22, 2024

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented Apr 18, 2024

For element-hq/element-web#27326

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • Sign-off given on the changes (see CONTRIBUTING.md).

@turt2live
Copy link
Member Author

I believe the Element Web build error is failed (or lack of) branch matching to the react-sdk.

@turt2live turt2live marked this pull request as ready for review April 18, 2024 23:00
@turt2live turt2live requested a review from a team as a code owner April 18, 2024 23:00
@turt2live turt2live requested review from a team as code owners April 18, 2024 23:17
@turt2live turt2live removed request for a team and florianduros April 18, 2024 23:23
@turt2live
Copy link
Member Author

turt2live commented Apr 18, 2024

Sorry for the spam here. As is hopefully obvious from the commit history, I tried to use @types/serviceworker and that promptly exploded in my face.

My vote is to give up for now and put a help wanted label on it.

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

I don't really know why we're indirecting via window in the first place. crypto is defined as a global property - ie, it should be available via globalThis in all environments.

I suspect it's because this code predates the existence of globalThis, so used to say crypto = window.crypto, which then got (incorrectly, imho) turned into crypto = globalThis.window.crypto.

Likewise TextEncoder.

@richvdh
Copy link
Member

richvdh commented Apr 22, 2024

My vote is to give up for now and put a help wanted label on it.

I don't really understand. If you're not going to keep working on the PR, could you close it please?

@turt2live
Copy link
Member Author

I'm not sure I understand the action requested here. Should the crypto indirection be removed?

As for closing the PR: I'm saying we give up on trying to get types to work, but the PR is required to work. I cannot commit to debugging the entire type system much further, so a help wanted issue feels like the correct approach.

@richvdh
Copy link
Member

richvdh commented Apr 22, 2024

I'm not sure I understand the action requested here. Should the crypto indirection be removed?

I'm suggesting that, rather than doing anything special for service workers, you change lines 19-21 to be:

export let crypto = globalThis.crypto;
export let subtleCrypto = crypto?.subtle ?? crypto?.webkitSubtle;  // or just `crypto?.subtle`
export let TextEncoder = globalThis.TextEncoder;

... and be done.

As for closing the PR: I'm saying we give up on trying to get types to work, but the PR is required to work. I cannot commit to debugging the entire type system much further, so a help wanted issue feels like the correct approach.

oh right. Well, that was far from obvious from your comment :)

@turt2live turt2live requested a review from richvdh April 22, 2024 18:05
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

LGTM, though needs an update to the PR title.

@turt2live turt2live changed the title Force service worker-safe crypto when operating under a service worker Modernize window.crypto access constants Apr 22, 2024
@turt2live turt2live added this pull request to the merge queue Apr 22, 2024
Merged via the queue into develop with commit c09da9a Apr 22, 2024
27 of 28 checks passed
@turt2live turt2live deleted the travis/msc3916 branch April 22, 2024 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants