Create public import for uniqueId helper

[TechieQian]

Adds public import for uniq-id

#20165

[bertdeblock]

I think we also need to add an entry here?

[chriskrycho]

Thanks again for working on this! We talked about it today at the Framework Core Team meeting, and noted that there were a couple gaps between the RFC which introduced unique-id and the actual implementation:

• the introduction of plain old functions as helpers via RFC #0756: Default Helper Manager
• the lack of an import in support of template strict mode and first-class component templates
• the lack of a clear definition of

These are not your responsibility to address as part of this change, however, so we’ve come up with a path that provides the public import we need for <template> support, as outlined in the rest of this review.

If you’re interested, we need to do some work separately to solve those issues, by writing a small RFC which defines how we should approach helpers like this going forward, not least so they can be usable as normal JS functions without the extra bits you'll see below. Let me know if you’re interested in that: @wycats is available to help get that written!

Again, though, the only things we need to do to land this are to address the comments in the review. Thanks for doing this!

[chriskrycho]

Here, then, you should be able to use those new imports like this:

Suggested change

	let first = uniqueId();
	let second = uniqueId();
	let first = getValue(invokeHelper({}, uniqueId()));
	let second = getValue(invokeHelper({}, uniqueId()));

The rest of the test should “just work” once you’ve added that in. This gets at the bit we’ll need to do follow-up work around—this obviously isn’t a great experience in JS! However, we don’t need to tackle that here, and it needs some broader design work!

[TechieQian]

Hrm, assuming we want just uniqueId since its not fn. So trying
getValue(invokeHelper({}, uniqueId) I get the following very intentional sounding error.

Died on test #1: Found a helper manager, but it was an internal built-in helper manager. `invokeHelper` does not support internal helpers yet.

I dug into @glimmer/runtime a bit, and it seems like we just determine this to be an internal built-in helper manager based on the fact that it takes in a type function.

  function invokeHelper(context, definition, computeArgs) {
    ... 

    if (true
    /* DEBUG */
    && typeof internalManager === 'function') {
      throw new Error('Found a helper manager, but it was an internal built-in helper manager. `invokeHelper` does not support internal helpers yet.');
    }

Here's the minimal repro of the situation:

   let myRef = internalHelper(() => createConstRef('foo', 'unique-id'));
    try {
      invokeHelper({}, myRef);
    } catch (ex) {
      console.error('see error:', ex);
    }

I am missing some context of what an internal helper is vs external(?). Had this just been a Reference, I see there are helper functions like valueForRef(createConst('foo', 'unique-id')) // => foo

[chriskrycho]

Sorry, forgot about this in the midst of a bunch of other things—just kicked back off CI for it and will take another review pass later today!

[TechieQian]

Sorry, forgot about this in the midst of a bunch of other things—just kicked back off CI for it and will take another review pass later today!

Hey @chriskrycho , pls let me know how we should proceed here. As it stands, I dont think we could just use invokeHelper per my comment on the suggestion. Happy to dig further!

[chriskrycho]

Yeah, I noted that and will bring it up with folks next week! Thanks for checking in. Just had end-of-quarter extra stuff these past couple weeks.

[wycats]

I met with @TechieQian and we discussed the bulk of the open issues here.

I took some (very incomplete and possibly cryptic) notes during the meeting: https://gist.github.com/wycats/6800716eb6744f63d9b9396670d5c625

The next steps are to meet about this in a spec meeting. @TechieQian is available to attend whenever the discussion is on the agenda. It's a pretty meaty topic, so we'll want to allocate plenty of time to discuss it.

[wycats]

One observation that we made: since "functions as helpers" is now the law of the land, it basically works to export the uniqueId JS function and have people use it in <template>.

However, we'd need to be sure that we guarantee that when a function with no autotracking dependencies is used as a helper, it's never spuriously re-evaluated. I believe that this is the case today, but we'd need to be really sure.

[bertdeblock]

Looks good AFAICT!

[chriskrycho]

1. This needs a rebase.
2. Last time we were here, CI was failing, and there were open questions about how to get it the rest of the way across the line. It was not clear to me from @wycats' comment above whether it's actually unblocked here or not.

[TechieQian]

Will happily rebase and continue when 2. Is resolved.

…

On Wed, Feb 15, 2023 at 12:59 PM Chris Krycho ***@***.***> wrote: 1. This needs a rebase. 2. Last time we were here, CI was failing, and there were open questions about how to get it the rest of the way across the line. It was not clear to me from @wycats <https://github.com/wycats>' comment above whether it's actually unblocked here or not. — Reply to this email directly, view it on GitHub <#20171 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADQ4LZAOTAF2SOMFIJZLSULWXUKR3ANCNFSM576MSXVA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[NullVoxPopuli]

I believe that this is the case today, but we'd need to be really sure.

I have never observed behavior to the contrary -- functions with no consumed tracked data never re-evaluate (unless the parent context is torn down and re-rendered -- which would be expected, ofc)

Can we move forward with this PR? <3

[NullVoxPopuli]

I pushed up a rebase + some minor tweaks in #20464 -- thanks for getting this started @TechieQian !!! you're the best 🎉

[wagenet]

Closed by #20464.