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

Update DeepHavenJsApiLinker to allow exporting GWT JsTypes as ES6 module #2733

Merged

Conversation

vegegoku
Copy link
Contributor

No description provided.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 12, 2022

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@vegegoku
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@niloc132 niloc132 requested review from niloc132 and mofojed August 13, 2022 00:54
@niloc132 niloc132 added documentation Improvements or additions to documentation jsapi javascript Pull requests that update Javascript code labels Aug 13, 2022
@niloc132 niloc132 added this to the Aug 2022 milestone Aug 13, 2022
…es' into export-gwt-js-types-as-es6-modules

# Conflicts:
#	web/client-api/src/main/java/io/deephaven/web/DeephavenJsApiLinker.java
Copy link
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

Going to create tickets for some additional things, such as publishing these as a package in npmjs (e.g. @deephaven/jsapi and/or @deephaven/jsapi-internal).

Whitespacing is inconsistent in many of the new lines in these examples.

@niloc132 niloc132 linked an issue Aug 24, 2022 that may be closed by this pull request
@mofojed mofojed self-requested a review August 29, 2022 15:54
@mofojed
Copy link
Member

mofojed commented Sep 7, 2022

Spoke with @niloc132 about this, and the change looks good as a halfway point, however it makes consuming the library more difficult. Ultimately suggest we keep this as a fork or a separate branch until we have the full thing exported as a module (dh-internal as well) and published to npmjs.

The reason why this is difficult is because the Web UI still needs to load it remotely from the server. Currently, it's just doing a regular script tag with type text/javascript, which add dh to the global namespace: https://github.com/deephaven/web-client-ui/blob/1d10956499cd451145e704690bc8f2d5cfb6a786/packages/code-studio/public/index.html#L41
There is then the jsapi-shim which pulls dh from the global namespace, which we use from our modules to be able to import the jsapi: https://github.com/deephaven/web-client-ui/blob/1d10956499cd451145e704690bc8f2d5cfb6a786/packages/jsapi-shim/src/dh.ts#L11

That shim only works if dh is already on the global namespace. However, when importing a script using type="module", it defers loading until after the page is already loaded. So we'd need to defer the web UIs startup entirely to wait for the dh module to import before we load the rest of our code. It's simply a pain in the butt, and doesn't really have any benefits yet at this point. We also can't just do import dh from './dh-core.js', since it's loading from a remote address (e.g. we need to do <script type="module" src="http://localhost:10000/jsapi/dh-core.js">). As a consumer of the library, this halfway point is actually more difficult to use/consume (even though it is a step in the right direction).

Once dh-internal is also a module, and they're both published to npmjs, theoretically should be able to change jsapi-shim to just:

import dh from '@deephaven/jsapi';
export default dh;

And everything should work. (We could also just change all our imports from import dh from '@deephaven/jsapi-shim' to import dh from '@deephaven/jsapi', but by keeping the shim we can actually have the same code work on both Enterprise and Community until we get Enterprise to export as a module as well).

So, I approve these changes, but please don't merge them to main, and keep a separate long running branch for this development :).

@niloc132 niloc132 added the ReleaseNotesNeeded Release notes are needed label Feb 7, 2023
@niloc132 niloc132 merged commit ad97a9e into deephaven:main Feb 9, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Feb 9, 2023
@deephaven-internal
Copy link
Contributor

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
DocumentationNeeded javascript Pull requests that update Javascript code jsapi ReleaseNotesNeeded Release notes are needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JS API should be built as an ES6 module
4 participants