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 array-helper] Implement array helper RFC #318 #17054

Merged
merged 1 commit into from Oct 13, 2018

Conversation

Projects
None yet
4 participants
@josemarluedke
Copy link
Contributor

commented Oct 8, 2018

This implements the array helper.
RFC: emberjs/rfcs#318

Closes #17052.

@rwjblue
Copy link
Member

left a comment

Looking good! I left some pretty nit-picky comments inline (sorry about that), but also would like to see a few more tests:

  • Confirming that passing an array result into a component invocation, and that component using {{#each properly rerenders/updates as items change
  • Checking that the output of array follows the "immutable" style by having a component that we control which is passed an array and we inspect it for === equality.

@josemarluedke josemarluedke force-pushed the josemarluedke:jl-array-helper branch from a86b037 to 4ef1df2 Oct 8, 2018

@josemarluedke

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2018

@rwjblue I have applied the requested changes as well added a few more tests.

Please take another round of review.

@rwjblue

rwjblue approved these changes Oct 9, 2018

@rwjblue rwjblue requested a review from mmun Oct 9, 2018

@rwjblue

This comment has been minimized.

Copy link
Member

commented Oct 9, 2018

LGTM, I also pinged @mmun for review...

@josemarluedke josemarluedke force-pushed the josemarluedke:jl-array-helper branch from 4ef1df2 to 0fab6f4 Oct 9, 2018

@chancancode

This comment has been minimized.

Copy link
Member

commented Oct 10, 2018

Should we debug freeze the array? The empty case is frozen like hashes iirc

@rwjblue

This comment has been minimized.

Copy link
Member

commented Oct 10, 2018

Should we debug freeze the array? The empty case is frozen like hashes iirc

Yes, probably.

@chancancode

This comment has been minimized.

Copy link
Member

commented Oct 10, 2018

Unfortunately debug freezing is not going to be super easy. We can merge this as-is and do a follow up PR for that. I also just realized that the current version of the glimmer code seems to always return a new array/object even for the empty case.

We can do something like:

class DebugFreezeReference<T> implements PathReference<T> {
  constructor(private inner: PathReference<T>) {
    this.tag = inner.tag;
  }

  value(): T {
    return Object.freeze(this.inner.value());
  }

  get(path: string): PathReference<Opaque> {
    return this.inner.get(path); // or should this be `new DebugFreezeReference(this.inner.get(path))`?
  }
}

function array(_vm: VM, args: Arguments): PathReference<Opaque[]> {
  return new DebugFreezeReference(args.positional.capture());
}

function hash(_vm: VM, args: Arguments): PathReference<Dict<Opaque>> {
  return new DebugFreezeReference(args.named.capture());
}

@josemarluedke josemarluedke force-pushed the josemarluedke:jl-array-helper branch from 0fab6f4 to 7cc684b Oct 10, 2018

@josemarluedke josemarluedke force-pushed the josemarluedke:jl-array-helper branch from 7cc684b to d6d9526 Oct 10, 2018

@chancancode

This comment has been minimized.

Copy link
Member

commented Oct 11, 2018

Looking good. Probably should have a few more test testing "capturing" the array into JS values and make sure it is correct. Something like this:

let captured;

this.registerHelper('capture', function([array]) {
  captured = array;
  return "captured";
});

this.render(`{{capture (array ...)}}`);

assert.deepEqual(captured, [...]);

this.runTask(this.set(...));

assert.deepEqual(captured, [...]);

...

Basically what you already did in the last test, but more coverage around the actual values, and that it updates correctly, etc. You can probably just merge that into your last test since it's already doing similar things.

@josemarluedke josemarluedke force-pushed the josemarluedke:jl-array-helper branch from d6d9526 to 59bd071 Oct 12, 2018

@josemarluedke

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2018

I have addressed the last few comments from @chancancode. This should be ready for another review.

@chancancode

This comment has been minimized.

Copy link
Member

commented Oct 12, 2018

@josemarluedke have you tried the indentation style in #17054 (comment)? If the linter prefer you write it like this, it's nbd

@josemarluedke

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2018

@chancancode Yes I did, I changed most of the code to that indentation style. I did not change all because some already had a line break due to component args. But if you do think I should change that as well, I'm happy to do so.

@rwjblue rwjblue merged commit 94c8118 into emberjs:master Oct 13, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rwjblue

This comment has been minimized.

Copy link
Member

commented Oct 13, 2018

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.