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

Add catchSafeObjects option (default false) to no-get rule to catch get(foo, 'bar') #912

Merged
merged 1 commit into from Aug 13, 2020
Merged

Add catchSafeObjects option (default false) to no-get rule to catch get(foo, 'bar') #912

merged 1 commit into from Aug 13, 2020

Conversation

bmish
Copy link
Member

@bmish bmish commented Aug 13, 2020

Previously, this rule could only catch instances with the this object in scenarios like get(this, ...) or getProperties(this, ...). But we can safely catch and autofix other objects besides this too:

Before:

import { get } from '@ember/object';
get(foo, 'bar');

After:

import { get } from '@ember/object';
foo.bar;

CC: @mongoose700

@bmish bmish added the Bug label Aug 13, 2020
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

This change seems good to me, note: that this is both breaking and a bugfix. SemVer allows for such a thing, but its worth calling out in the changelog IMHO.

@bmish
Copy link
Member Author

bmish commented Aug 13, 2020

Planning to move this behind a new rule option...will update this PR.

…f the imported get/getProperties functions on non-this objects
@bmish bmish changed the title Update the no-get rule to catch usages of the imported get/getProperties functions on non-this objects Add catchSafeObjects option (default false) to no-get rule to catch usages of the imported get/getProperties functions on non-this objects Aug 13, 2020
@bmish bmish added Enhancement and removed Bug labels Aug 13, 2020
@bmish
Copy link
Member Author

bmish commented Aug 13, 2020

Updated to move this behind a catchSafeObjects (default false) option that I will enable in the next major release.

I also plan to add another option catchUnsafeObjects (which will always default to false) for catching foo.get('bar') which likely involves an Ember Object but we can't be sure about it.

@bmish bmish merged commit 9334f3a into ember-cli:master Aug 13, 2020
@rwjblue
Copy link
Member

rwjblue commented Aug 13, 2020

Gotcha, seems good to me. 😄

@bmish
Copy link
Member Author

bmish commented Aug 13, 2020

Follow-up PR to add catchUnsafeObjects option: #913

@bmish bmish changed the title Add catchSafeObjects option (default false) to no-get rule to catch usages of the imported get/getProperties functions on non-this objects Add catchSafeObjects option (default false) to no-get rule to catch get(foo, 'bar') Aug 14, 2020
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.

None yet

2 participants