Skip to content

Make Rtrace ESM by default, with UMD fallback #1515

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

Merged
merged 5 commits into from
Oct 22, 2020
Merged

Make Rtrace ESM by default, with UMD fallback #1515

merged 5 commits into from
Oct 22, 2020

Conversation

dcodeIO
Copy link
Member

@dcodeIO dcodeIO commented Oct 22, 2020

Does what #1513 did for the loader for RTrace as well.

  • I've read the contributing guidelines

@dcodeIO dcodeIO requested a review from MaxGraey October 22, 2020 15:25
@dcodeIO dcodeIO changed the title Make RTrace ESM by default, with UMD fallback Make Rtrace ESM by default, with UMD fallback Oct 22, 2020
this.decrementCount = 0; // The following hooks cannot just be on the prototype but must be
// bound so the Rtrace instance can be used as a WebAssembly import.

this.onalloc = this.onalloc.bind(this);
Copy link
Member

@MaxGraey MaxGraey Oct 22, 2020

Choose a reason for hiding this comment

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

How about use arrow functions instead bindings?
instead this.load_val_i32 = this.load_val_i32.bind(this) just use later:

load_val_i32 = (id, value) => {
   return value;
}

and for rest methods use the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean moving all of the declarations into the ctor like that?

Copy link
Member

@MaxGraey MaxGraey Oct 22, 2020

Choose a reason for hiding this comment

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

No, just remove this.load_val_i32 = this.load_val_i32.bind(this) and friends and bind this via arrow function during declaration like:

class RTrace {

   ....
  onresize = () => { 
      ...
  }
  load_val_i32 = (id, value) => {
     return value;
  }
  ....
}

instead:

class RTrace {
   constructor() {
       this.onresize = this.onresize.bind(this);
       this.load_val_i32 = this.load_val_i32.bind(this);
   }
   ....
   onresize() {
       ....
   }
   load_val_i32(id, value) {
     return value;
  }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, how would that look for

  load_ptr(id, bytes, offset, address) {
    this.accessShadow(address + offset, bytes, true);
    return address;
  }

when this is accessed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also note that binding the functions makes them usable without a proper this context, i.e. as a Wasm import that naturally loses the this context. Wondering if that'd still work when not binding.

Copy link
Member

Choose a reason for hiding this comment

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

instance methods defined via arrow function grabbed current context (this of class or object). It's typical alternative for this.method = this.method.bind(this)

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm

SyntaxError: unknown: Support for the experimental syntax 'classProperties' isn't currently enabled (179:11):

  177 |   // Runtime instrumentation
  178 |
> 179 |   onalloc = (ptr) => {

Copy link
Member

@MaxGraey MaxGraey Oct 22, 2020

Choose a reason for hiding this comment

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

Hmm, class properties should work since V8 7.2 or even earlier

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think that's a Babel-ish error. May I suggest to make this a follow-up? Otherwise sounds good if it works!

Copy link
Member

Choose a reason for hiding this comment

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

For babel you should just add this: https://babeljs.io/docs/en/babel-plugin-syntax-class-properties. It add only syntax for parser, but don't do actually transformation (lowering)

@dcodeIO dcodeIO merged commit 2e249e4 into master Oct 22, 2020
@dcodeIO dcodeIO deleted the rtrace-esm branch June 1, 2021 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants