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

refactor(decorators): Refactor to use ember-argument-decorators #51

Merged
merged 1 commit into from
Oct 27, 2017

Conversation

pzuraq
Copy link
Collaborator

@pzuraq pzuraq commented Oct 26, 2017

Updates to the latest ember-decorators and refactors the addon to use ember-argument-decorators. Also includes the ember-legacy-class-transform to make classes work the same in modern and legacy versions of Ember.

No breaking changes here, mostly maintenance.

@pzuraq pzuraq force-pushed the refactor/ember-argument-decorators branch from bab621b to 15994de Compare October 27, 2017 03:33
@pzuraq pzuraq force-pushed the refactor/ember-argument-decorators branch from 15994de to f1f84db Compare October 27, 2017 04:41
@pzuraq pzuraq merged commit dce99c4 into master Oct 27, 2017
@pzuraq pzuraq deleted the refactor/ember-argument-decorators branch October 27, 2017 05:02
@pzuraq
Copy link
Collaborator Author

pzuraq commented Oct 27, 2017

Been seeing that flakiness for the test that failed on master, not sure what's causing it but we should try to track it down.

@pzuraq
Copy link
Collaborator Author

pzuraq commented Oct 27, 2017

Also, looks like the @type(Element) descriptor breaks fastboot. I need to think on this, this is going to be a common problem in fastboot with ember-argument-decorators.

@simonihmig
Copy link
Collaborator

@pzuraq The type assertion added here was a breaking change for me, because targetwas at times undefined. As it can be null, is there a reason undefined should be forbidden?

@pzuraq
Copy link
Collaborator Author

pzuraq commented Nov 1, 2017

So, my guess here is that you have a parent component that does something like this:

{{#ember-popper
  target=target
}}

{{/ember-popper}}

You're just passing forward default values, and if no value was passed in by the parent it's undefined, so it gets set (unintentionally) and the null value doesn't ever get set, and we end up triggering this bug.

It's a really common bug/pattern, not unique to us (see ember-power-select for instance) and it's more apparent for things that should never be null or undefined, like placement for instance. I'm thinking there should be another decorator that allows users to default if the field is undefined for this reason:

@argument
@defaultIfUndefined
target = null;

alternatively, you could argue that this really should be the default behavior, and users should have to opt out:

@argument
target = target in this ? this.target : null; // Key is defined, value exists but it was set to `undefined` purposefully, so pass it through

I'm still kind of bikeshedding about which solution @argument should take, so I'm definitely ok with adding undefined to the union type for now to prevent this from happening

@simonihmig
Copy link
Collaborator

You're just passing forward default values, and if no value was passed in by the parent it's undefined, so it gets set (unintentionally) and the null value doesn't ever get set, and we end up triggering this bug.

Well, yeah, different cases... something like that for some tests, where I tested for different non-popper related things and did not care to pass something meaningful as target. In another example I have some "registration" protocol between parent and child components, but this happens in the next run loop, so at first the target element (which is passed to ember-popper) is undefined...

I'm still kind of bikeshedding about which solution argument should take, so I'm definitely ok with adding undefined to the union type for now to prevent this from happening

I think I fixed the issues for now (Travis still running, but seem to pass). So no hurry, but I guess other users could run into this as well...

@pzuraq
Copy link
Collaborator Author

pzuraq commented Nov 6, 2017

Ended up going with the following API:

@argument({ defaultIfUndefined: true })

I think adding this to each of ember-popper's arguments would make sense since it is a low level component, in general. What do you think @kybishop?

@kybishop
Copy link
Owner

kybishop commented Nov 6, 2017

@pzuraq sounds good to me

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.

3 participants