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

make webclient a npm package #123

Merged
merged 10 commits into from
Aug 3, 2023
Merged

make webclient a npm package #123

merged 10 commits into from
Aug 3, 2023

Conversation

toloudis
Copy link
Contributor

@toloudis toloudis commented Jul 18, 2023

Final tweaks to publish the webclient as a npm package that other js projects can use. Bring version number in sync with Agave release and python client (all in lockstep).
There may be some more tweaks upcoming, while testing the gh action to ensure that everything publishes correctly.
The main client code includes a change to allow better error handling when failing to connect.

@toloudis toloudis marked this pull request as draft July 18, 2023 00:19
@toloudis toloudis marked this pull request as ready for review July 18, 2023 20:56
@toloudis toloudis requested a review from a team as a code owner July 18, 2023 20:56
Comment on lines +34 to +42
"@types/chai": "^4.3.5",
"@types/dat.gui": "^0.7.10",
"@types/mocha": "^10.0.1",
"@types/three": "^0.152.1",
"@typescript-eslint/eslint-plugin": "^5.60.1",
"@typescript-eslint/parser": "^5.60.1",
"acorn": "^8.8.2",
"babel-loader": "^9.1.2",
"chai": "^4.3.7",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these types and package versions match?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They often get out of sync especially with patch releases. I selected "most current" at this time.

}

isReady(): boolean {
return this.socket.readyState === 1;
return this.socket!.readyState === 1;
Copy link
Contributor

@ShrimpCryptid ShrimpCryptid Jul 19, 2023

Choose a reason for hiding this comment

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

This and a few other lines have a warning from the linter (Forbidden non-null assertion). Ideally either change the statement or change the linter rules for the project!

ShrimpCryptid
ShrimpCryptid previously approved these changes Jul 19, 2023
Copy link
Contributor

@ShrimpCryptid ShrimpCryptid left a comment

Choose a reason for hiding this comment

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

Looks good, added a few minor comments!

console.warn("connection failed. refresh to retry.");
}, 3000);
};

Choose a reason for hiding this comment

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

Would you want this code to have a try/catch that tries to reestablish connection before asking the user to refresh?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for now there aren't really any known use cases where this should happen. This is really just some placeholder code (there's a TODO above). I'll simplify it even more.

"name": "agave-webclient",
"version": "1.0.0",
"name": "@aics/agave-webclient",
"version": "1.5.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to have this auto-update with new release versions, or does this have to be manually updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there's a version bump script (tbump.toml) that goes to all the files that have these version number in them and updates all of them

@@ -105,6 +105,7 @@ export class CommandBuffer {

const command = this.prebuffer[i];
const commandCode = command[0] as string;
//console.log("AGAVE." + commandCode, command.slice(1));
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure - I'll remove it

ShrimpCryptid
ShrimpCryptid previously approved these changes Aug 2, 2023
Copy link
Contributor

@ShrimpCryptid ShrimpCryptid left a comment

Choose a reason for hiding this comment

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

Looks good! Left a few notes :)

@toloudis toloudis merged commit 17c6622 into main Aug 3, 2023
@toloudis toloudis deleted the feature/js-api branch August 3, 2023 22:27
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