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

Update argument handling #23

Merged
merged 2 commits into from
May 6, 2020
Merged

Update argument handling #23

merged 2 commits into from
May 6, 2020

Conversation

chancancode
Copy link
Member

@chancancode chancancode commented Apr 29, 2020

This commit updates the argument handling:

  • To match RFC PR #620
    • Returns null on null or undefined
    • Use development mode assertions for other invalid arguments
    • Add test for modifiers usage
    • Add test for ...attributes usage
  • To match more closely the likely Ember side implementation
    • Handle argument errors at runtime
    • owner.hasRegistration('helper:element') === true

Since the argument errors are moved to runtime, this also removes the need for separate node tests. Previously, that also caused an error when ember-auto-import is included. Since we removed the qunit dependency we can now include ember-auto-import to align with the default addon blueprint.

See also https://discordapp.com/channels/480462759797063690/485447409296736276/704799818316251216

Copy link
Collaborator

@cibernox cibernox left a comment

Choose a reason for hiding this comment

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

LGTM. Is this something that might be ever be bundled in glimmer VM itself or is it too dynamic?

@chancancode
Copy link
Member Author

@cibernox that's the plan! I think it just so happens that no one has the cycle to work on the actual implementation yet, probably partly due to this polyfill being good enough, so I guess that's somewhat my fault :P

@chancancode chancancode force-pushed the arguments-handling branch 2 times, most recently from 373cf3a to 1b1a2ca Compare April 29, 2020 22:59
@chancancode chancancode force-pushed the arguments-handling branch 3 times, most recently from c69dd03 to 76e1737 Compare April 30, 2020 06:55
@chancancode chancancode force-pushed the arguments-handling branch 5 times, most recently from 28fcb78 to b0b3a9b Compare May 6, 2020 03:19
This commit updates the argument handling:

* To match [RFC PR #620](emberjs/rfcs#620)
  * Returns `null` on `null` or `undefined`
  * Use development mode assertions for other invalid arguments
  * Add test for modifiers usage
  * Add test for `...attributes` usage
* To match more closely the likely Ember side implementation
  * Handle argument errors at runtime
  * `owner.hasRegistration('helper:element') === true`

Since the argument errors are moved to runtime, this also removes
the need for separate node tests. Previously, that also caused an
error when `ember-auto-import` is included. Since we removed the
`qunit` dependency we can now include `ember-auto-import` to align
with the default addon blueprint.

See also https://discordapp.com/channels/480462759797063690/485447409296736276/704799818316251216

Closes #6
@chancancode chancancode merged commit 38fe9e3 into master May 6, 2020
@chancancode chancancode deleted the arguments-handling branch May 6, 2020 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants