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

Check for undefined attributeBindings #901

Merged
merged 2 commits into from
Aug 18, 2021
Merged

Conversation

kpfefferle
Copy link
Member

When we deploy our application using v3.0.2-3, we see the following console error:

TypeError: Cannot read property 'includes' of undefined

The stack trace points to

if (this.attributeBindings.includes('data-test-row-count')) {
which was introduced in #893

This PR adds an additional condition to check that this.attributeBindings is defined before calling .includes() on it. This fixes the error we see in our deployed application.

@kpfefferle kpfefferle requested a review from mixonic August 11, 2021 14:42
@kpfefferle kpfefferle changed the title Check for undefined attribtueBindings Check for undefined attributeBindings Aug 11, 2021
@kpfefferle kpfefferle force-pushed the kpfefferle/fix-deploy-error branch from 55b2bfa to 0f5367f Compare August 11, 2021 15:13
@mixonic
Copy link
Member

mixonic commented Aug 13, 2021

@kpfefferle is this for Ember 4.0? Can you ensure we have coverage of a reproduction?

@kpfefferle
Copy link
Member Author

@mixonic This happened in an app running Ember 3.27. This is the sort of thing that would likely be caught in a typed codebase as TS would notice that you are calling .includes on a property that could be undefined.

Do you have a suggestion how to test this case besides running our test suite against a production build (which then would drop the data-test-* selectors that many of our tests rely on?).

@mixonic
Copy link
Member

mixonic commented Aug 17, 2021

@kpfefferle we can use a production build of Ember but force the data-test- selectors to be enabled. We're actually doing something similar for internal Addepar codebases.

Let me take a stab at adding it to the build on this PR. I'd really like to make sure this has coverage.

@shaun-addepar
Copy link

shaun-addepar commented Aug 18, 2021

Hi, is there any update on this? We have a couple of blocker bugs in beta and staging that prevents all tables from rendering data and looks like it could be fixed by this PR.

@mixonic mixonic force-pushed the kpfefferle/fix-deploy-error branch from c276600 to 3ad7737 Compare August 18, 2021 19:17
@mixonic mixonic force-pushed the kpfefferle/fix-deploy-error branch from 3ad7737 to 4423bdf Compare August 18, 2021 19:20
@mixonic
Copy link
Member

mixonic commented Aug 18, 2021

I was unable to write a test for this as ember-test-selectors itself causes attributeBindings to be present even if using a production builds for test.

I added a comment explaining the patch and how it relates to tests, and I also did add a production build to the test suite here because why not.

@mixonic mixonic merged commit 57dd8b3 into master Aug 18, 2021
@mixonic mixonic deleted the kpfefferle/fix-deploy-error branch August 18, 2021 19:27
@mixonic
Copy link
Member

mixonic commented Aug 18, 2021

Included in 3.0.2 🎉

Arijit-Roy pushed a commit to xomaczar/ember-table that referenced this pull request Sep 6, 2021
Co-authored-by: Matthew Beale <matt.beale@addepar.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants