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

allow non-Symbol object properties on RSAA #192

Merged

Conversation

darthrellimnad
Copy link
Contributor

@darthrellimnad darthrellimnad commented Jun 8, 2018

Here's the proposed change regarding Issue #188.

Dev setup was easier than expected and worked just as described! 👍Let me know if I need to change anything for convention, or add/edit additional tests or documentation.

This change only removes this requirement from the validateRSAA method, and from the documentation. It doesn't seem as if anything else in the codebase requires the old behavior.

This is still a breaking change though and does effect the existing RSAA specification, so probably best grouped into a future major (v3+) release after ppl have time to discuss in this PR (and if we still want to make this change) :)

@nason
Copy link
Collaborator

nason commented Jun 8, 2018

Thanks @darthrellimnad! This looks good to me.

Like I said in the issue, I don't know why this was part of the RSAA spec. At the same time, I don't see the harm in changing it.

Let's leave this open for a few days so people can chime in. If no issues are raised I'm happy to merge this in as-is 👍

@nason nason requested a review from agraboso June 8, 2018 19:06
@nason nason added this to the 3.0.0 milestone Jun 10, 2018
@nason
Copy link
Collaborator

nason commented Jun 10, 2018

@darthrellimnad I just cut a new next branch for a 3.0 release. Can you update this PR to target that branch instead?

@darthrellimnad darthrellimnad changed the base branch from master to next June 12, 2018 06:07
@darthrellimnad darthrellimnad force-pushed the major/issue-188-custom-rsaa-props branch from 1e6cef3 to d374151 Compare June 12, 2018 06:19
@coveralls
Copy link

coveralls commented Jun 12, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling d374151 on darthrellimnad:major/issue-188-custom-rsaa-props into 3a19b1b on agraboso:next.

@darthrellimnad
Copy link
Contributor Author

darthrellimnad commented Jun 12, 2018

@nason I rebased with next and fixed a merge conflict with the test file. Looked like 2 changes each used the action29 case, so I changed my added Symbol prop test to action30. Let me know if you need anything else 👍

@nason
Copy link
Collaborator

nason commented Jun 12, 2018

Awesome, thanks! I'm going to go ahead and merge this into next so I can publish a beta later today.

@nason nason merged commit 8664c11 into agraboso:next Jun 12, 2018
@nason
Copy link
Collaborator

nason commented Jun 14, 2018

This has been released in 3.0.0-beta.0

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

Successfully merging this pull request may close these issues.

3 participants