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

Helpers mutating hash may error if no hash was provided. #14189

Closed
rwjblue opened this issue Sep 2, 2016 · 5 comments
Closed

Helpers mutating hash may error if no hash was provided. #14189

rwjblue opened this issue Sep 2, 2016 · 5 comments
Labels

Comments

@rwjblue
Copy link
Member

rwjblue commented Sep 2, 2016

One optimization that the Glimmer engine does is to avoid creating objects when it does not need to. An example of this can be seen when evaluating helper and/or component invocations where there is no hash arguments. Such as this:

{{page-title "My App"}}

In order to avoid creating a wasted hash argument here, Glimmer will use a custom EVALUATED_EMPTY_NAMED_ARGS which always returns a shared constant named EMPTY_DICT. In order to prevent that shared constant from being mutated, the engine uses Object.freeze(EMPTY_DICT).

All of these assumptions/steps are completely reasonable. Unfortunately, where this falls down is when a helper (such as ember-page-title) mutate that provided hash argument. If you do this, the following error is triggered:

TypeError: Can't add property foo, object is not extensible
    at https://output.jsbin.com/solokun:55:12
    at Function.create (https://s3.amazonaws.com/builds.emberjs.com/canary/ember.debug.js:12837:22)
    at https://s3.amazonaws.com/builds.emberjs.com/canary/ember.debug.js:8301:69
    at CompiledHelper.evaluate (https://s3.amazonaws.com/builds.emberjs.com/canary/ember.debug.js:42855:20)
    at VM.evaluateOperand (https://s3.amazonaws.com/builds.emberjs.com/canary/ember.debug.js:50094:40)
    at PutValueOpcode.evaluate (https://s3.amazonaws.com/builds.emberjs.com/canary/ember.debug.js:45494:16)
    at VM.execute (https://s3.amazonaws.com/builds.emberjs.com/canary/ember.debug.js:50066:28)
    at Template.render (https://s3.amazonaws.com/builds.emberjs.com/canary/ember.debug.js:49616:23)
    at RootState.render (https://s3.amazonaws.com/builds.emberjs.com/canary/ember.debug.js:9969:61)
    at runInTransaction (https://s3.amazonaws.com/builds.emberjs.com/canary/ember.debug.js:20692:28)

JSBin Demo Here


We need to decide if we must "fix" this by providing a custom hash object to each helper invocation (less than ideal because we would much prefer to benefit from doing less work).

@rwjblue
Copy link
Member Author

rwjblue commented Sep 2, 2016

@chancancode - Need your thoughts on this...

@ef4
Copy link
Contributor

ef4 commented Sep 2, 2016

I'm not feeling super sympathetic on this one. Mutating your arguments is usually a bad idea.

It's unfortunate that we can't easily provide consistent behavior: either always give you a frozen hash (whether or not it's empty), or always give you a copy that is yours to abuse. Either of those choices sound expensive in terms of allocations.

@rwjblue
Copy link
Member Author

rwjblue commented Sep 2, 2016

@ef4 - Perhaps we could freeze/seal all params and hash when in debug only? That would provide a fairly common footing but still avoid the extra cost in production...

@chancancode
Copy link
Member

Yeah. I think I agree with @ef4. The debugSeal idea seems good if it's not very slow?

@Turbo87
Copy link
Member

Turbo87 commented Sep 8, 2016

FWIW the problem in ember-page-title has been resolved already: ember-cli/ember-page-title#41

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants