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(618): handle remaining wasm panics, and expose is_valid_flux() API #621

Merged
merged 5 commits into from
Dec 15, 2022

Conversation

wiedld
Copy link
Contributor

@wiedld wiedld commented Dec 14, 2022

Closes #618

Done:

  • wasm start now triggers logger setting at the wasm root (on load in browser). Confirmed will allow logging in LspServer and utils module.
  • update test fixtures, to make more explicit what the parse() contract does.
    • note: we are misusing parse() as a validation method in the UI. Which is wrong.
  • expose an is_valid_flux() method which can be used for validation in the UI

Payload comparison:

The size of our wasm can impact network load times. With the changes made here, the release build has an increase in 25kB size.
Screen Shot 2022-12-13 at 10 34 31 PM

#[wasm_bindgen]
pub fn initLog() {
#[cfg(feature = "console_log")]
console_log::init_with_level(log::Level::Trace)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Chose wasm-logger over console-log lib, due to the reported build size.

@wiedld wiedld marked this pull request as ready for review December 14, 2022 07:05
@wiedld wiedld requested a review from a team as a code owner December 14, 2022 07:05
@wiedld wiedld requested review from appletreeisyellow and removed request for a team December 14, 2022 07:05
Copy link
Contributor

@appletreeisyellow appletreeisyellow left a comment

Choose a reason for hiding this comment

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

Thanks for adding the test case! I have one question below

@@ -18,7 +18,7 @@ lto = true
default = ["cmd"]
strict = []
cmd = ["clap", "simplelog", "tokio", "tower-service", "lspower/runtime-tokio"]
wasm = ["futures", "js-sys", "lspower/runtime-agnostic", "tower-service", "wasm-bindgen", "wasm-bindgen-futures"]
wasm = ["futures", "js-sys", "fluxlang", "lspower/runtime-agnostic", "tower-service", "wasm-bindgen", "wasm-bindgen-futures"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm very new to wasm. Just trying to understand. What does fluxlang do here? To expose is_valid_flux() API? I thought is_valid_flux() is already included in wasm-bindgen.

Copy link
Contributor Author

@wiedld wiedld Dec 14, 2022

Choose a reason for hiding this comment

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

is_valid_flux() uses the lang module, which is not included in the wasm without this feature.

But then did a build size sanity check, to make sure the wasm is still an ok size.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it! Thanks

@wiedld
Copy link
Contributor Author

wiedld commented Dec 14, 2022

So...lesson learned.
Although the #[wasm_bindgen(start)] annotation runs in our test suite, that is because the test suite is on target node. It does NOT work when imported to the browser, because that is built using target browser and then transformed via webpack.

chain of actions:

  • The #[wasm_bindgen(start)] annotation runs the start method once on module eval (import).
  • When building to the bundler target, the resultant compiled code is wasm.__wbindgen_start();
  • Then webpack transforms this code into WEBPACK_IMPORTED_MODULE_1__.__wbindgen_start(), where the annotated method is called on module import.
  • BUT --> it does not get called. And the wasm silently dies (does nothing else).

reason for failure?

  • We are building our wasm to target bundler, which has no sideEffects.
  • No side effects --> webpack only looks for imported functions, and will not run eval on the imported module (a.k.a. will never call the start method).
  • Observed behavior:
    • #[wasm_bindgen(start)]
      --> LSP silently does nothing.
    • #[wasm_bindgen(start)] + sideEffect: true
      --> webpage silently died on load
    • change UI to @influxdata/flux-lsp-node
      --> clearly fails, since node targets have fs, etc
    • build with -t web + UI using WasmPackPlugin
      --> will hit a double declaration of the start method init (bug also seen in many other open wasm-pack issues)

Screen Shot 2022-12-14 at 1 37 50 PM

.

So what is the conclusion? Do it the manual way:

  • go back to setting the logger on LspServer creation.
  • call the init() method manually, and globally, on import into the UI.

@wiedld
Copy link
Contributor Author

wiedld commented Dec 14, 2022

Fixed.

On version bump in UI, we then need to update this file to:

  1. init the log
    import {
      parse as flux_parse,
      format_from_js_file as flux_format_from_js_file,
      initLog,
    } from '@influxdata/flux-lsp-browser'
    
    initLog()
    
  2. the exported parse() should also run is_flux_valid() first

…le import. Instead, provide the initLog to manually call on import in UI.
@@ -16,7 +16,6 @@ case $BUILD_MODE in
;;
"dev")
BUILD_FLAG="--dev"
BUILD_MODE_ARGS="${BUILD_MODE_ARGS},console_log"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we no longer include the console_log just for dev build, we don't need to re-bind the BUILD_MODE_ARGS

Copy link
Contributor

@appletreeisyellow appletreeisyellow left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation!

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.

Wasm panics on logging.
2 participants