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

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

Open
kenchan13579 opened this issue Jun 24, 2020 · 7 comments
Open

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

kenchan13579 opened this issue Jun 24, 2020 · 7 comments

Comments

@kenchan13579
Copy link

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

'you must pass a path to {{set}}',

@chriskrycho
Copy link

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
Copy link

bachvo commented Jun 24, 2020

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
Copy link
Author

kenchan13579 commented Jun 24, 2020

@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
Copy link
Contributor

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
Copy link
Owner

pzuraq commented Oct 14, 2020

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
Copy link

I strongly prefer it. 👍

@patocallaghan
Copy link
Contributor

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.

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

No branches or pull requests

5 participants