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

feature/loader #190

Merged
merged 20 commits into from
Nov 8, 2024
Merged

feature/loader #190

merged 20 commits into from
Nov 8, 2024

Conversation

joshua-dean
Copy link
Collaborator

@joshua-dean joshua-dean commented Oct 2, 2024

Loader

Closes #25.

Description

  • Added overlay with spinning loading icon
    • Added during init and removed once loading is complete
  • Pulled a lot of init logic into it's own file
  • Added a staggered_ulabel_init which displays the screen earlier and adds a configurable delay between each step. This is intended to help visualize/debug the loading process, and demonstrate features like the loader
    • This branch uses the "staggered" init with a 250ms delay per step. This is currently hard-coded but we could make this a feature flag or configurable.
  • Added build-dev-and-demo npm script
  • Added a bunch of missing init functions to ULabel's type declaration

The "staggered" init is async to allow the delays to run. The ULabel.load_image_promise wasn't playing nicely with async, so I opted to use .decode() instead (why wasn't this done in the first place?).

I resisted the urge to completely rearrange and init process in this PR, but now there is a place to do so if we want to optimize in the future.

The production build for this is way slower than previously, which I suspect is due to the static function calls (index.js and initializer.ts both call each other so there's some resolution that webpack has to handle). I'm going to leave this as a draft until I can solve this, or we decide that we don't care.
Production build size issues solved by #200 and #201.

PR Checklist

  • Merged latest main target branch
  • Version number in package.json has been bumped since last release
  • Version numbers match between package package.json and src/version.js
  • Ran npm install and npm run build AFTER bumping the version number
  • Updated documentation if necessary (currently just in api_spec.md)
  • Added changes to changelog.md

Breaking API Changes

maybe

@joshua-dean joshua-dean self-assigned this Oct 2, 2024
@joshua-dean joshua-dean added the enhancement New feature or request label Oct 2, 2024
@joshua-dean
Copy link
Collaborator Author

joshua-dean commented Oct 2, 2024

Yeah so the really long builds is a result of webpack increasing the size of dist/ every build, because initializer.ts calls static ULabel functions in index.js, so it's a somewhat circular dependency. It's possible there's a setting to make webpack not do this.

To avoid this, we would need to pull has_night_mode_cookie, initialize_annotation_canvases, load_image_promise, create_listeners (massive), after_init, and elvl_fatal to not be in index.js.

Static class functions that take an instance of the class seem problematic in our use case anyways, so hoisting these elsewhere is probably in our best interests. I might punt on this specific feature and do some PRs on the other ones (like create_listeners) separately.

This was referenced Oct 7, 2024
@joshua-dean joshua-dean changed the base branch from main to org/other-static-fns October 7, 2024 19:59
@joshua-dean
Copy link
Collaborator Author

I might punt on this specific feature and do some PRs on the other ones (like create_listeners) separately.

The necessary changes to avoid the issue are in #200 and #201. I have merged the latest of #201 into this branch and made it the base for this PR. I have tested this and am able to build repeatedly without dist/ ballooning in size.

@joshua-dean
Copy link
Collaborator Author

#200 and #201 are approved, so I merged the latest and am marking this as "ready for review".

@joshua-dean joshua-dean marked this pull request as ready for review October 10, 2024 14:18
Copy link
Member

@TrevorBurgoyne TrevorBurgoyne left a comment

Choose a reason for hiding this comment

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

loader is very cool, and i like the general init refactor

index.d.ts Show resolved Hide resolved
src/loader.ts Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
@joshua-dean joshua-dean mentioned this pull request Oct 11, 2024
6 tasks
This ensures the style is properly removed
when the div is removed
@joshua-dean joshua-dean mentioned this pull request Nov 1, 2024
@joshua-dean
Copy link
Collaborator Author

@TrevorBurgoyne I consolidated to a single init (the async one) without any delays, and merged the latest main through the whole chain.

#200 and #201 are approved, so if this is approved I'll merge them all into #202 so we can release them all together.

Copy link
Member

@TrevorBurgoyne TrevorBurgoyne left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Base automatically changed from org/other-static-fns to rc/v0.16.0 November 8, 2024 18:24
@joshua-dean joshua-dean merged commit e35f83f into rc/v0.16.0 Nov 8, 2024
1 check passed
@joshua-dean joshua-dean deleted the feature/loader branch November 8, 2024 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should have a loader
2 participants