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 catchEvents option (default false) to no-side-effects rule #916

Merged
merged 1 commit into from Aug 18, 2020
Merged

Add catchEvents option (default false) to no-side-effects rule #916

merged 1 commit into from Aug 18, 2020

Conversation

bmish
Copy link
Member

@bmish bmish commented Aug 15, 2020

Inspired by the ember-best-practices/no-side-effect-cp rule.

Catches side-effect-causing function calls like:

  • this.send()
  • this.sendAction()
  • this.sendEvent()
  • this.trigger()

CC: @mongoose700

}

/**
* Finds:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this also find this.foo?.bar?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess so, updated.

{
// Imported sendEvent function:
code:
'import { sendEvent } from "@ember/object/events"; computed(function() { sendEvent(); })',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you import it under a different name, to make sure it's not assuming it's named sendEvent?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added.

@bmish bmish changed the title Update no-side-effects rule to catch event functions inside computed properties Add catchEvents option (default false) to no-side-effects rule Aug 18, 2020
@bmish
Copy link
Member Author

bmish commented Aug 18, 2020

Moved new functionality behind a catchEvents rule option to avoid a breaking change.

@bmish bmish merged commit 809456f into ember-cli:master Aug 18, 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