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] Upstream built-in helpers and modifiers from Ember #1245

Merged
merged 1 commit into from
Dec 18, 2020

Conversation

pzuraq
Copy link
Member

@pzuraq pzuraq commented Dec 18, 2020

Upstreams the built-in helpers and modifiers from Ember:

  • on
  • fn
  • array
  • hash
  • get
  • concat

Tests were adapted directly from the existing test suites in Ember, with omissions wherever particular test functionality did not make sense (e.g. integration testing get with <Input/>).

Breaking Changes

  • setPath is now required on the global context

@rwjblue
Copy link
Member

rwjblue commented Dec 18, 2020

Current failure is:

not ok 1393 Chrome 87.0 - [2 ms] - [integration] jit :: {{on}} Modifier: SUPPORTS_EVENT_OPTIONS is correct (private API usage)
    ---
        actual: >
            null
        stack: >
                at _loop (http://localhost:7357/153953091774/tests/assets/glimmer-vm.js:18481:21)
                at http://localhost:7357/153953091774/tests/assets/glimmer-vm.js:18491:11
                at suite (http://localhost:7357/153953091774/tests/assets/glimmer-vm.js:18493:9)
                at jitSuite (http://localhost:7357/153953091774/tests/assets/glimmer-vm.js:18407:12)
                at Module.callback (http://localhost:7357/153953091774/tests/assets/tests.js:13379:20)
                at Module.exports (http://localhost:7357/153953091774/tests/assets/loader.js:106:32)
                at requireModule (http://localhost:7357/153953091774/tests/assets/loader.js:27:18)
                at http://localhost:7357/153953091774/tests/index.html?hidepassed:139:11
        message: >
            Expected 0 assertions, but 1 were run
        negative: >
            false
        browser log: |

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

  • Why are the various test files empty?
    • packages/@glimmer/integration-tests/test/helpers/array-test.ts
    • packages/@glimmer/integration-tests/test/helpers/fn-test.ts
    • packages/@glimmer/integration-tests/test/helpers/get-test.ts
    • packages/@glimmer/integration-tests/test/helpers/hash-test.ts
    • packages/@glimmer/integration-tests/test/helpers/concat-test.ts
    • packages/@glimmer/integration-tests/test/helpers/array-test.ts

* @param obj The object provided to get a value from
* @param path The path to get the value from
*/
export let setPath: (obj: object, path: string, value: unknown) => unknown;
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious why we'd actually need setPath, wouldn't all sets end up being setProp (I thought TWB for {{whatever someArg=foo.bar.baz}} we'd be calling setProp for bar['baz'] instead of Ember.set(foo, 'bar.baz'))?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is specifically for {{get}}. In general, when we see a path in Glimmer, we break it down into a chain of references. However, when the path is dynamic, we cannot do that, we have to do it all within a single reference. We already had getPath because there was an instance of this before for getting a path (the key on each) but we never encountered a setPath before. {{get}} can also update the value that is accessed (similar to how mut works), so that's why we need the ability to set a path - it's dynamic and not broken down into pieces.

Copy link
Member

Choose a reason for hiding this comment

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

So to clarify, its for (mut (get being used together (since I didn't think that {{something whatever=(get otherThing 'lol.path')}} allowed mutation of otherThing.lol.path`)? Is that right?

packages/@glimmer/runtime/lib/helpers/get.ts Show resolved Hide resolved
packages/@glimmer/runtime/lib/helpers/get.ts Outdated Show resolved Hide resolved
packages/@glimmer/util/lib/untouchable-this.ts Outdated Show resolved Hide resolved
@pzuraq pzuraq force-pushed the upstream-built-in-helpers-and-modifiers branch from ad9dec5 to f4b315f Compare December 18, 2020 15:47
@pzuraq pzuraq force-pushed the upstream-built-in-helpers-and-modifiers branch 5 times, most recently from e75450d to a433a62 Compare December 18, 2020 21:38
Upstreams the built-in helpers and modifiers from Ember:

- `on`
- `fn`
- `array`
- `hash`
- `get`
- `concat`

\## Breaking Changes

- `setPath` is now required on the global context
@pzuraq pzuraq force-pushed the upstream-built-in-helpers-and-modifiers branch from a433a62 to 7e3ef01 Compare December 18, 2020 21:49
@pzuraq pzuraq merged commit 51d2991 into master Dec 18, 2020
@pzuraq pzuraq deleted the upstream-built-in-helpers-and-modifiers branch December 18, 2020 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants