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: JS API shouldn't require reference to window object #6193

Merged
merged 8 commits into from
Oct 21, 2024

Conversation

niloc132
Copy link
Member

@niloc132 niloc132 commented Oct 11, 2024

Also introduces a dh.Event type to use instead of the browser Event and CustomEvent types.

Fixes #5537
Fixes #6190

@niloc132 niloc132 added this to the 0.38.0 milestone Oct 11, 2024
@niloc132 niloc132 requested a review from bmingles October 11, 2024 16:48
@niloc132 niloc132 self-assigned this Oct 11, 2024
@niloc132 niloc132 marked this pull request as draft October 11, 2024 16:48
@niloc132 niloc132 force-pushed the 5537-jsapi-window-reference branch from cd82643 to 730e756 Compare October 17, 2024 19:42
@niloc132 niloc132 force-pushed the 5537-jsapi-window-reference branch from 730e756 to 26020ba Compare October 17, 2024 19:44
@niloc132 niloc132 marked this pull request as ready for review October 17, 2024 19:45
@niloc132 niloc132 force-pushed the 5537-jsapi-window-reference branch from 26020ba to fbdc2a8 Compare October 17, 2024 19:51
@niloc132 niloc132 requested a review from mofojed October 18, 2024 15:58
@bmingles
Copy link
Contributor

I've tested this in the vscode extension by removing most of the polyfill we needed before. Remaining polyfill is:

import ws from 'ws';

export function polyfillDh(): void {
  /* @ts-ignore */
  global.self = global;
  /* @ts-ignore */
  global.this = global;

  global.WebSocket = ws as unknown as (typeof global)['WebSocket'];
}

I think we can get rid of the global.self + global.this assignments by using globalThis in js api.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/globalThis

@mofojed any opinions on this?

metadata.set(key, info.getOptions().headers.get(key));
return null;
});
return authUpdate().then(ignore -> Promise.resolve(Boolean.TRUE));
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have to use Promise.resolve in the return? I thought non-promise and promise returns from a then both result in the same thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

In JS yes, but in Java you cannot define a method signature to return two unrelated types - union types effectively don't exist in this fashion (and while technically jvm bytecode allows method signatures to be specified based on return type, the java language doesn't have support for that). So, when we want to return a value directly, we must always wrap in Promise.resolve(...).

@niloc132 niloc132 requested a review from bmingles October 18, 2024 20:31
Copy link
Contributor

@bmingles bmingles left a comment

Choose a reason for hiding this comment

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

LGTM.

@niloc132 niloc132 merged commit 680047b into deephaven:main Oct 21, 2024
17 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Core+ JsApi client option to connect over websockets jsapi - improvements for 3rd party consumers
2 participants