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

Use assert() for "read-only attributeBindings" warning #254

Merged
merged 1 commit into from
Oct 12, 2018

Conversation

Turbo87
Copy link
Collaborator

@Turbo87 Turbo87 commented Oct 12, 2018

This is similar to #205 and makes the warnings/error more consistent.

I'm marking this as a breaking change because it might cause previously succeeding builds to fail now. If the component has no data-test- properties everything should still work fine though.

@Turbo87 Turbo87 requested a review from mansona October 12, 2018 12:04
Copy link
Member

@mansona mansona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me 🎉

I understand what this is doing but I don't really understand the full context of the why this change is needed 🤔I see that you have marked this as breaking but I wonder if we have a process to write up a release notes snippet to explain why this has changed to our consumers.

@Turbo87
Copy link
Collaborator Author

Turbo87 commented Oct 12, 2018

@mansona the explanation is in #204 and the reasoning seemed to make sense to me, which is why I merged #205. but then I later saw that only one of the warnings was converted, so this PR here is finishing the conversion process.

@Turbo87 Turbo87 merged commit c65ae33 into mainmatter:master Oct 12, 2018
@Turbo87 Turbo87 deleted the assert branch October 12, 2018 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants