[RFC #338] Implement new template helpers by cibernox · Pull Request #17177 · emberjs/ember.js

cibernox · 2018-11-02T23:39:25Z

Open questions:

The {{gt}}, {{gte}}, {{lt}} and {{lte}} from ember-truth-helpers take a named argument forceNumber=true to transform input arguments to numbers. Do we want to replicate those too?

- [x] Implement `{{eq}}` helper - [ ] Implement `{{not}}` helper - [ ] Implement `{{and}}` helper - [ ] Implement `{{or}}` helper - [ ] Implement `{{gt}}` helper - [ ] Implement `{{gt3}}` helper - [ ] Implement `{{lt}}` helper - [ ] Implement `{{lte}}` helper

cibernox · 2018-11-03T23:39:34Z

packages/@ember/-internals/glimmer/tests/integration/helpers/gt-test.js

+      this.render(`{{gt left right}}`, {
+        left: undefined,
+        get right() {
+          debugger;


@rwjblue Any guidance on how to define a getter or a CP to verify that its value is not eagerly pulled by the helper? Seems that right now the keys of this object are eagerly accessed by Object.assign by the setup of the test itself.

Instead of rendering gt directly render a component whose own template uses gt, then have that component have left and right CP’s that you can track when they are evaluated

It has lazy evaluation of the second argument, so it doesn't even get read if the first argument is already undefined. For now without an option to coerce strings to numbers.

cibernox · 2018-11-04T14:04:12Z

@rwjblue Tests are failing because of missing documentation, however I've documented every helper, so I'm not sure what I'm missing.

Turbo87 · 2018-11-04T14:20:08Z

packages/@ember/-internals/glimmer/lib/helpers/and.ts

+  @public
+  @method and
+  @for Ember.Templates.helpers
+  @since 2.7.0


That seems wrong ;)

Master is 2.7, am I wrong? 2.6 maybe?

do you mean 3.7?

packages/@ember/-internals/glimmer/lib/helpers/and.ts

packages/@ember/-internals/glimmer/lib/helpers/gt.ts

packages/@ember/-internals/glimmer/lib/helpers/or.ts

… as 0

packages/@ember/-internals/glimmer/lib/helpers/or.ts

rwjblue · 2018-11-06T00:22:18Z

Do we want to replicate those too?

Off the top of my head, I’d say no....

rwjblue

Looking really good here, only major thing that I see is that it needs feature flagging...

rwjblue · 2018-11-06T00:23:19Z

packages/@ember/-internals/glimmer/lib/resolver.ts

 const BUILTINS_HELPERS = {
  if: inlineIf,
  action,
+  and,


Adding to the BUILTINS_HELPERS array needs to be guarded by feature flags

cibernox · 2018-11-06T11:55:26Z

I added a feature flag. It's enabled by default.

packages/@ember/-internals/glimmer/lib/helpers/and.ts

packages/@ember/-internals/glimmer/lib/helpers/eq.ts

packages/@ember/-internals/glimmer/lib/helpers/gt.ts

packages/@ember/-internals/glimmer/lib/helpers/gte.ts

packages/@ember/-internals/glimmer/lib/helpers/not-eq.ts

packages/@ember/-internals/glimmer/lib/helpers/not.ts

packages/@ember/-internals/glimmer/lib/helpers/or.ts

packages/@ember/-internals/glimmer/lib/helpers/eq.ts

packages/@ember/canary-features/index.ts

rwjblue · 2018-11-06T13:05:48Z

packages/@ember/-internals/glimmer/lib/helpers/and.ts

+  let last: any = true;
+  for (let i = 0; i < references.length; i++) {
+    last = references[i].value();
+    if (!last) {


@chancancode - Should we consider [] as falsey here? In general, in template land I think we do right?

yeah, I think we do.

ember.js/packages/@ember/-internals/glimmer/lib/utils/to-bool.ts

Lines 4 to 10 in 1b7e613

export default function toBool(predicate: Opaque): boolean {

if (isArray(predicate)) {

return (predicate as { length: number }).length !== 0;

} else {

return !!predicate;

}

}

I bigger question is should we support proxies, I completely forgot about that. I think that needed to be discussed and clarified in the RFC 🤧

That is surprising. I was expecting and/or helpers to behave just like && / II. Why this deviation from standard JS behaviour?

{{#each @posts as |post|}} ... {{else}} No posts! {{/each}}

{{#each}} is designed to display lists, and seems normal that it has special behavior for empty lists, but and and or are not for lists, so I don't see why the behavior of {{each}} should influence them and make them behave different than && and ||

On top of that, this is not how ember-truth-helpers behave. I'm not saying that we can't deviate from what e-t-h if there is a good reason, but it would be nice not to.

Well it really isn't about each, {{#if someEmptyArray}}{{else}}{{/if}} will render the falsey branch. I think it is very confusing to have if behave differently than eq/not.

It works the same way for...

{{#if @posts as |post|}} ... {{else}} No posts! {{/if}}

...and with, let etc. I think the bottom line is it would be pretty surprising if {{else}} doesn't work consistently in the control flow things in the "language".

So now what if you refactor from the first example into...

{{#each (or @posts @reblogs) as |post|}} ... {{else}} No posts! {{/each}}

...what should this do?

I think it is probably important for the "language" to have one boolean semantics, and for better or for worse empty arrays are falsey everywhere else.

rwjblue · 2018-11-06T13:07:52Z

packages/@ember/-internals/glimmer/lib/helpers/gt.ts

+    return false;
+  }
+  let right = references[1].value();
+  if (right === undefined) {


Does this satisfy a specific edge case? I would have imagined that the following would have been enough:

return left > right;

I tried, but typescript doesn't like comparing things with null or undefined.

rwjblue · 2018-11-06T13:09:12Z

packages/@ember/-internals/glimmer/lib/helpers/gte.ts

+    return false;
+  }
+  if (right === null) {
+    return left >= 0;


This seems odd to me, why do we need to extra logic (vs just using the final return left >= right)?

Same reason. Typescript doesn't ket me compare things with undefined or null. How can we silence that?

Co-Authored-By: cibernox <miguel.camba@gmail.com>

packages/@ember/canary-features/index.ts

cibernox · 2020-01-15T14:37:36Z

Closing this as it would have to be reworked

locks · 2020-01-15T14:43:37Z

For reference, the original RFC has been superseded. See emberjs/rfcs#388 (comment) for full details.

cibernox changed the title ~~[RFC #338] Implement new template helpers~~ [WIP][RFC #338] Implement new template helpers Nov 2, 2018

Implement {{not}} helper

95925ab

cibernox commented Nov 3, 2018

View reviewed changes

cibernox force-pushed the rfc-338/new-template-helpers branch 3 times, most recently from 9703584 to c21356b Compare November 4, 2018 10:48

Implement gt helper

4fecacb

It has lazy evaluation of the second argument, so it doesn't even get read if the first argument is already undefined. For now without an option to coerce strings to numbers.

cibernox force-pushed the rfc-338/new-template-helpers branch from c21356b to 4fecacb Compare November 4, 2018 10:50

cibernox added 4 commits November 4, 2018 11:53

Implement {{tl}} helper

98e99e1

Implement {{gte}} and {{lte}} helpers

207ea8c

Implement {{and}} helper

22a8d33

Implement {{or}} helper

7142979

cibernox changed the title ~~[WIP][RFC #338] Implement new template helpers~~ [RFC #338] Implement new template helpers Nov 4, 2018

cibernox mentioned this pull request Nov 4, 2018

Add new basic helpers to Ember emberjs/rfcs#388

Closed

Turbo87 reviewed Nov 4, 2018

View reviewed changes

makepanic reviewed Nov 4, 2018

View reviewed changes

packages/@ember/-internals/glimmer/lib/helpers/gt.ts Show resolved Hide resolved

makepanic reviewed Nov 4, 2018

View reviewed changes

packages/@ember/-internals/glimmer/lib/helpers/or.ts Outdated Show resolved Hide resolved

cibernox added 2 commits November 4, 2018 17:51

Perf improvement: replace [].reduce with for loop

58f1426

Ensure gt, get, lt, and lte compare treat null on the second argument…

552f776

… as 0

makepanic reviewed Nov 4, 2018

View reviewed changes

packages/@ember/-internals/glimmer/lib/helpers/or.ts Show resolved Hide resolved

cibernox added 2 commits November 6, 2018 00:36

Implement {{not-eq}} helper

d024e91

Fix linting problems

1d791ad

rwjblue requested changes Nov 6, 2018

View reviewed changes

Add feature flag for new template helpers

77fc37a

locks suggested changes Nov 6, 2018

View reviewed changes

fix linting errors

544da6d

rwjblue reviewed Nov 6, 2018

View reviewed changes

locks and others added 7 commits November 6, 2018 14:13

Update packages/@ember/-internals/glimmer/lib/helpers/or.ts

b6f5f7f

Co-Authored-By: cibernox <miguel.camba@gmail.com>

Update packages/@ember/-internals/glimmer/lib/helpers/not.ts

c720549

Co-Authored-By: cibernox <miguel.camba@gmail.com>

Update packages/@ember/-internals/glimmer/lib/helpers/not-eq.ts

4ccf90a

Co-Authored-By: cibernox <miguel.camba@gmail.com>

Update packages/@ember/-internals/glimmer/lib/helpers/gte.ts

8fbdda2

Co-Authored-By: cibernox <miguel.camba@gmail.com>

Update packages/@ember/-internals/glimmer/lib/helpers/gt.ts

9f8e8a5

Co-Authored-By: cibernox <miguel.camba@gmail.com>

Update packages/@ember/-internals/glimmer/lib/helpers/eq.ts

f3e4192

Co-Authored-By: cibernox <miguel.camba@gmail.com>

Update packages/@ember/-internals/glimmer/lib/helpers/and.ts

81e61d7

Co-Authored-By: cibernox <miguel.camba@gmail.com>

locks suggested changes Nov 6, 2018

View reviewed changes

packages/@ember/canary-features/index.ts Outdated Show resolved Hide resolved

Add @category ember-basic-template-helpers to docs of all new helpers

1b7e613

locks closed this Jan 15, 2020

	export default function toBool(predicate: Opaque): boolean {
	if (isArray(predicate)) {
	return (predicate as { length: number }).length !== 0;
	} else {
	return !!predicate;
	}
	}

Uh oh!

Conversation

cibernox commented Nov 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cibernox commented Nov 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

locks Nov 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rwjblue commented Nov 6, 2018

Uh oh!

rwjblue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cibernox commented Nov 6, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cibernox Nov 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cibernox Nov 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cibernox commented Jan 15, 2020

Uh oh!

locks commented Jan 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

cibernox commented Nov 2, 2018 •

edited

Loading

locks Nov 6, 2018 •

edited

Loading

cibernox Nov 6, 2018 •

edited

Loading

cibernox Nov 6, 2018 •

edited

Loading