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.

Sign up for GitHub

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

Create public import for uniqueId helper #20171

Closed
wants to merge 8 commits into from
Closed

Create public import for uniqueId helper #20171

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8a53d0a
re-export uniqid
Aug 29, 2022
861ae89
Adding test and also re-organizing the export
Aug 30, 2022
a7e4fea
Correcting to single quotes
Aug 30, 2022
d864d4b
correct test description
Aug 30, 2022
541b3d9
adding empty line to adhere to linting
Aug 30, 2022
ffbbe6c
address lint
Sep 1, 2022
9a54122
place on global
Sep 1, 2022
b0aadba
addressed comments to use invokeHelper
Sep 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/@ember/-internals/glimmer/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,3 +324,4 @@ export {
setComponentManager,
} from './lib/utils/managers';
export { isSerializationFirstNode } from './lib/utils/serialization-first-node-helpers';
export { uniqueId } from './lib/helpers/unique-id';
TechieQian marked this conversation as resolved.
Show resolved Hide resolved
2 changes: 1 addition & 1 deletion packages/@ember/-internals/glimmer/lib/helpers/unique-id.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export default internalHelper((): Reference<string> => {
// implementation details into an intimate API. It also ensures that the UUID
// always starts with a letter, to avoid creating invalid IDs with a numeric
// digit at the start.
function uniqueId() {
export function uniqueId() {
TechieQian marked this conversation as resolved.
Show resolved Hide resolved
// @ts-expect-error this one-liner abuses weird JavaScript semantics that
// TypeScript (legitimately) doesn't like, but they're nonetheless valid and
// specced.
Expand Down
28 changes: 27 additions & 1 deletion packages/@ember/-internals/glimmer/tests/integration/helpers/unique-id-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,34 @@
import { RenderingTestCase, strip, moduleFor, runTask } from 'internal-test-helpers';
import {
AbstractTestCase,
RenderingTestCase,
strip,
moduleFor,
runTask,
} from 'internal-test-helpers';
import { setProperties } from '@ember/object';
import { uniqueId } from '@ember/helper';
TechieQian marked this conversation as resolved.
Show resolved Hide resolved
import { EMBER_UNIQUE_ID_HELPER } from '@ember/canary-features';

if (EMBER_UNIQUE_ID_HELPER) {
moduleFor(
'Helpers test: {{unique-id}} JS',
class JSRenderingTest extends AbstractTestCase {
chriskrycho marked this conversation as resolved.
Show resolved Hide resolved
constructor(assert) {
super(assert);
this.assert = assert;
}
['@test it can be invoked as a JS function']() {
let first = uniqueId();
let second = uniqueId();
Copy link
Contributor

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

@TechieQian TechieQian Sep 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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


this.assert.notStrictEqual(
first,
second,
`different invocations of uniqueId should produce different values`
);
}
}
);
moduleFor(
'Helpers test: {{unique-id}}',
class extends RenderingTestCase {
Expand Down
2 changes: 2 additions & 0 deletions packages/@ember/helper/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,3 +314,5 @@

export { setHelperManager, helperCapabilities as capabilities } from '@glimmer/manager';
export { invokeHelper, hash, array, concat, get, fn } from '@glimmer/runtime';

export { uniqueId } from '@ember/-internals/glimmer';
2 changes: 2 additions & 0 deletions packages/ember/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import require, { has } from 'require';
import { getENV, getLookup, setLookup } from '@ember/-internals/environment';
import * as utils from '@ember/-internals/utils';
import { Registry, Container } from '@ember/-internals/container';
import { uniqueId } from '@ember/-internals/glimmer';
TechieQian marked this conversation as resolved.
Show resolved Hide resolved
import * as instrumentation from '@ember/instrumentation';
import { meta } from '@ember/-internals/meta';
import * as metal from '@ember/-internals/metal';
Expand Down Expand Up @@ -427,6 +428,7 @@ Ember._concat = concat;
Ember._get = get;
Ember._on = on;
Ember._fn = fn;
Ember._uniqueId = uniqueId;
TechieQian marked this conversation as resolved.
Show resolved Hide resolved

Ember._helperManagerCapabilities = helperCapabilities;
Ember._setHelperManager = setHelperManager;
Expand Down
1 change: 1 addition & 0 deletions packages/ember/tests/reexports_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ let allExports = [
['_hash', '@ember/helper', 'hash'],
['_get', '@ember/helper', 'get'],
['_concat', '@ember/helper', 'concat'],
['_uniqueId', '@ember/helper', 'uniqueId'],

// @ember/object
['Object', '@ember/object', 'default'],
Expand Down