pzuraq / ember-simple-set-helper

MIT License
12 stars 4 forks source link

When the helper is prefixed with addon name, it throws error #11

Open kenchan13579 opened 4 years ago

kenchan13579 commented 4 years ago

We prefix external addon helpers or components with the addon name. We passed ember-simple-set-helper$set this.state true. but it threw the error from this line https://github.com/pzuraq/ember-simple-set-helper/blob/6485ddaa5801ce15f9fa827efbd9f952a5bbc984/addon/helpers/set.js#L8

chriskrycho commented 4 years ago

Specifically, the prefixing is supported using https://github.com/rwjblue/ember-holy-futuristic-template-namespacing-batman. That should be distinct from the assertion thrown here—did it only appear when when you invoke with the namespace? Or is the problem with the helper itself, requiring a path?

bachvo commented 4 years ago

I believe the proper syntax for using ember-simple-set-helper is {{ember-simple-set-helper$set this "state" true}}. I can see the resemblance from the javascript set util side which is quite similar to this helper version set(this, 'state', true).

I think the issue is that the docs should have better examples of the proper syntax for developers to follow because they give examples like {{set this.foo "bar"}} when it should be {{set this "foo" "bar"}} (ignoring prefixing path for this example)

kenchan13579 commented 4 years ago

@chriskrycho Correct, it failed only when it's prefixed with namn space.

It could be that the transform doesn't take the namespace into the case. https://github.com/pzuraq/ember-simple-set-helper/blob/33e5fe80860b1393355771a5b983c588cbb7bc2b/lib/simple-set-transform.js

patocallaghan commented 4 years ago

Thanks @bachvo I've been tearing my hair out and actually came to open an issue and saw this. I've been using {{set this.foo "bar"}} in an in-repo-addon component and it was throwing that you must pass a path to {{set}} error. I'm guessing it's the same issue?

@pzuraq If it's a bug I have a reproduction app here which shows it failing (either visit the index route or there are tests). If on the other hand this is expected behaviour and we need update the docs, I'd be happy to put together a PR to do that.

pzuraq commented 4 years ago

At this point, I think we should just remove the transform altogether. It's difficult to maintain and it's causing a lot more grief than help. Without the transform, the API would be:

{{set this "foo" "bar"}}

Which is altogether not horrible. What does everyone think?

chriskrycho commented 4 years ago

I strongly prefer it. 👍

patocallaghan commented 4 years ago

No major disagreement here if it makes it more maintainable. It's slightly less ergonomic but at least it will be more obvious to users how it works. I'd be happy to whip up a codemod to make migration easier.