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

unique-id helper #659

Merged
merged 4 commits into from
Oct 16, 2020
Merged

unique-id helper #659

merged 4 commits into from
Oct 16, 2020

Conversation

steveszc
Copy link

@steveszc steveszc commented Aug 25, 2020

text/0659-id-helper.md Outdated Show resolved Hide resolved
@joukevandermaas
Copy link

I think calling this helper {{id}} is a problem for backwards compatibility. If older components have a local property called id, it will break if the template didn't use this. yet. Example:

<button id={{id}}>my button</button>

This perfectly valid piece of code will now break, because {{id}} is now a helper. Of course, this can be addressed by changing the code:

<button id={{this.id}}>my button</button>

But I would argue this type of issue can be pretty hard to track down. Maybe something like unique-id would work better?

@sandstrom
Copy link
Contributor

I think a less generic/general name would be better.

For example, I think the previously proposed {{guid-for …}} name is better. But other names would work too, my main complaint is that {{id …}} is too general.

@bendemboski
Copy link
Collaborator

Agreed for both backwards compatibility and readability reasons. I'd propose {{unique-id}} as it's much less likely to conflict with anything else, and IMO pretty clearly describes what it does.

@bendemboski
Copy link
Collaborator

Oh, haha, just read #612 (comment), which I hadn't before. I guess that's corroboration that unique-id will be readable/intuitive because I came up with the same name on my own 😆

@steveszc
Copy link
Author

steveszc commented Aug 26, 2020

I think calling this helper {{id}} is a problem for backwards compatibility. If older components have a local property called id, it will break if the template didn't use this. yet. Example:

<button id={{id}}>my button</button>

This perfectly valid piece of code will now break, because {{id}} is now a helper. Of course, this can be addressed by changing the code:

<button id={{this.id}}>my button</button>

But I would argue this type of issue can be pretty hard to track down. Maybe something like unique-id would work better?

Yeah I agree this is a problem we need to solve, but I'm not sure picking a different name will actually solve the problem (though it may make it less likely to occur). Any other name we could come up with would have the same potential problem colliding with existing controller/component properties.

Admittedly, other names we might choose (eg. unique-id) may be less likely to to have this problem, but it is still valid and possible for an existing template to reference a property called unique-id.

The argument-less helper invocation proposed here may be problematic for this reason, and it will be complicated by template strict mode, which will require mandatory parens to invoke an argument-less helper.

@MelSumner
Copy link
Contributor

Admittedly, other names we might choose (eg. unique-id) may be less likely to to have this problem, but it is still valid and possible for an existing template to reference a property called unique-id.

I think this is something we could find out- let's do a code search in Ember Observer and ask the community to weigh in on it (via Discord, Twitter, etc). That way we could choose something that won't be problematic for most use cases.

What do you think?

@steveszc
Copy link
Author

steveszc commented Aug 26, 2020

@MelSumner

I think this is something we could find out- let's do a code search in Ember Observer and ask the community to weigh in on it (via Discord, Twitter, etc). That way we could choose something that won't be problematic for most use cases.

Looks like unique-id will indeed be safer. A few large add-ons use {{id}}, including ember-concurrency, ember-bootstrap, and a host of materialize and cardstack addons.
{{unique-id}} has 0 results on ember observer
{{id}} is found in 122 add-ons on Ember Observer

It also may be worth considering the names uid and guid for terseness, both of these names have only a single addon using them.

@bendemboski
Copy link
Collaborator

It also may be worth considering the names uid and guid for terseness

What's the benefit of terseness? I understand that some built-in helpers are pretty terse, but we have {{link-to}}, {{query-params}}, {{has-block}} and {{has-block-params}}, so I don't think something like {{unique-id}} would be an outlier. Also since it's generally expected to be used in a let statement, terseness seems even less important than, e.g. {{eq}} or {{get}} which are always going to be used in compound statements.

@luxzeitlos
Copy link

In any case I strongly agree with the comments that unique-id would be much better.

However I do like the idea to also have the AST transforming variant in ember itself. I currently have an addon that basically implements what is proposed as alternative here. And its IMHO much more ergonomic to not require additional nesting.

@sukima
Copy link

sukima commented Sep 11, 2020

This would save a lot of boilerplate for me. Without I do this quite frequently:

export default class MyComponent extends Component {
  guid = guidFor(this);
}
{{#let (concat this.guid "-input") as |id|}}
  <label for={{id}}>Name</label>
  <input id={{id}} …>
{{/let}}

🤔 I guess that's not so bad. 🤷

@steveszc steveszc changed the title id helper unique-id helper Oct 2, 2020
@steveszc
Copy link
Author

steveszc commented Oct 2, 2020

I've just updated this RFC to use the helper name unique-id instead of id, per suggestions from feedback.

@rwjblue
Copy link
Member

rwjblue commented Oct 2, 2020

👋 We discussed this in today's core team meeting. Overall we are 👍 on the direction here, but would like to see a couple of (hopefully) small tweaks:

  • Remove the for= API - This is mentioned in the alternatives section, we think that should be the default and we should not provide both forms of the API.
  • Remove reference to guidFor - I think this largely falls out from removing the for= API since I believe that is the only part that requires the concept of "object based stability".

Thanks again for pushing on this @steveszc, hopefully the last round of tweaks 🤞...

@steveszc
Copy link
Author

steveszc commented Oct 2, 2020

@rwjblue Thanks for taking the time to review with the core team!

👋 We discussed this in today's core team meeting. Overall we are 👍 on the direction here, but would like to see a couple of (hopefully) small tweaks:

* Remove the `for=` API - This is mentioned in the alternatives section, we think that should be the default and we should not provide both forms of the API.

If the default/only invocation option is the argument-less form, then I see two possible APIs that we'll need to choose between.

  1. {{unique-id}} is a helper that returns a new unique value upon every invocation. This necessitates the usage of let blocks for re-use of an id value within a template.
  2. {{unique-id}} is a "keyword" that returns the same value every time it is used within a template. This would be accomplished via an AST transform, as outlined in the alternatives section.

Did the core team meeting discussion assume that one of these two options would be the preferred API? It seems to me that this RFC will need to articulate which of those two APIs is being proposed.

@MelSumner
Copy link
Contributor

IIRC, we want to be explicit in the RFC that use of the helper will return a unique ID on every invocation, and that we were okay with let. There was some discussion about making it a keyword but that seemed more difficult and like it would have a non-zero impact on template imports.

@steveszc does that help?

@sandstrom
Copy link
Contributor

sandstrom commented Oct 7, 2020

Maybe I've missed something, but if all this helper does is return a unique id, why does it need to be in Ember core?

export default buildHelper((_args, _options) => {
  return Math.round((Math.random() * 10 ** 20)).toString(36);
});

I thought the original idea was to make id generation for form id/for pairs easier. That would have been useful! (I'm still positive to that idea)

Now it's just a vanilla JS function wrapped in an Ember helper, not too different from Ember.merge, Ember.k and other "small helpers". If that's all we're doing here, isn't it better to start out with an addon and see if anyone starts using it? Or make an adjustment to the docs (for example here) explaining how to solve this.

Spending core-team cycles/effort on a JS one-liner doesn't seem worthwhile to me.

@rwjblue
Copy link
Member

rwjblue commented Oct 7, 2020

@steveszc

{{unique-id}} is a helper that returns a new unique value upon every invocation. This necessitates the usage of let blocks for re-use of an id value within a template.

Yes, this is the form we want to go with.

@rwjblue
Copy link
Member

rwjblue commented Oct 7, 2020

@sandstrom

Maybe I've missed something, but if all this helper does is return a unique id, why does it need to be in Ember core?

Good question! Ultimately, it does not need any privileged access or other things that are only available internally (and therefore "need" to be in internals). The reason it is critical that we ship this by default (in one form or another) is that it is imperative that it be available as part of the main Ember programming model. I personally don't care if that is via an addon that is included in the default stack (and it may well turn into that when we can actually use template imports; we could add a thing like @ember/forms or something) or if it is directly included in the emberjs/ember.js repo. Unless I've missed something, this RFC also doesn't specifically care about that either.

@sandstrom
Copy link
Contributor

sandstrom commented Oct 7, 2020

@rwjblue Alright, I can see some benefits in shipping even the weaker version, but I would have preferred one that did more than simply generate a random string (the earlier proposal with uniqueness based on an object).

But it's no biggie 😄

@steveszc
Copy link
Author

steveszc commented Oct 7, 2020

@rwjblue @MelSumner I believe the core team feedback has now been addressed.

@MelSumner
Copy link
Contributor

We have discussed this in today's Framework Core Team meeting, and decided to move this RFC to Final Comment Period.

@hergaiety
Copy link
Contributor

Just voicing support for this RFC and the proposed helper. It'll be incredibly useful in the apps I work on every day and I'm glad to see it coming to the core framework. Thanks to everyone involved in getting it to this stage. 🎉

@rwjblue
Copy link
Member

rwjblue commented Oct 16, 2020

One thing we have to make sure to do is that when we land this feature that we explicitly add tests for the "real world snippets" that this RFC mentions (to guarantee that they work as specified), but I guess that we all assumed that anyways....

@rwjblue
Copy link
Member

rwjblue commented Oct 16, 2020

We chatted about this again in todays core team meeting, and are 👍 on moving forward!

@rwjblue rwjblue merged commit 325f963 into emberjs:master Oct 16, 2020
@chriskrycho
Copy link
Contributor

Does an installable polyfill for this exist yet?

@ctjhoa
Copy link
Contributor

ctjhoa commented Jan 24, 2021

@chriskrycho
I've published one
https://www.npmjs.com/package/ember-unique-id-helper-polyfill

@bartocc
Copy link

bartocc commented Jun 12, 2022

What is the implementation status of this RFC in the framework?

@NullVoxPopuli
Copy link
Sponsor Contributor

What is the implementation status of this RFC in the framework?

unique-id landed in 4.4: https://github.com/emberjs/ember.js/releases/tag/v4.4.0

@bartocc
Copy link

bartocc commented Jun 13, 2022

Thx a lot for the update @NullVoxPopuli

I always find it hard to find where/when a RFC has made it to the framework…

For example, in the actual rendered version of this RFC , I can see the following at the top

Capture d’écran 2022-06-13 à 10 30 16

Is this the right place to search for this info?

@MrChocolatine
Copy link
Contributor

@bartocc
Personally I keep an eye on these 2 URLs:

(viva la chocolatine ✌️)

@bartocc
Copy link

bartocc commented Jun 13, 2022

Thx @MrChocolatine , I do follow the blog and I had missed the new feature in 4.4, but the releases subscription is a great tool 👍

(viva la chocolatine ✌️)

😆

ef4 added a commit that referenced this pull request Dec 15, 2023
Advance RFC #659 "unique-id helper" to Stage Recommended
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.