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

Avoid usage of unguarded getRangeAt and add eslint rule #16212

Merged
merged 5 commits into from Jul 29, 2019

Conversation

@bor0
Copy link
Contributor

commented Jun 18, 2019

Description

Closes #15606.

This PR addresses #15606. I ran a grep through all getRangeAt calls and fixed all occurrences. It also adds a new eslint rule.

Types of changes

The PR introduces a Bug fix (non-breaking change which fixes an issue), and also adds a rule
enforcement to avoid similar problems in the future

@aduth
Copy link
Member

left a comment

Thanks for this proposal!

Each custom ESLint rule should have its own documentation:

https://github.com/WordPress/gutenberg/tree/master/packages/eslint-plugin/docs/rules

It should also be included in the "Rules" tab of the README.md file:

https://github.com/WordPress/gutenberg/tree/master/packages/eslint-plugin#rules

It's a fairly targeted rule, which makes me wonder if it's the sort of thing where we could generalize the guarding. That said, it's a pretty specific issue with this one function affecting some browsers and not others. I guess we'd really only care to revise it if there were other range functions which are similarly problematic when unguarded.

packages/eslint-plugin/configs/custom.js Outdated Show resolved Hide resolved
@bor0

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

Thanks @aduth, I updated the PR.

@aduth
aduth approved these changes Jun 26, 2019
Copy link
Member

left a comment

Looks great, thanks! 👍

@aduth

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

Some last-minute thoughts / feedback:

  • We should include a CHANGELOG entry detailing the rule being added (done in afc18a4).
  • I considered (and pushed in 2bf8129) that we name this as "no-" prefixed, rather than "avoid-", since we already have a fair number of theses "avoidance" rules which are consistently named as such. Let me know if you have any thoughts on this.

I was also wondering about the developer experience for someone who encounters this warning. Will it be obvious what they need to do to resolve the issue, when the message is simply "Avoid unguarded getRangeAt" ? I wonder if we could expand the message and/or implement a fixer (depending on difficulty).

It still looks good in its current form, and we could merge it as-is if you're okay with the proposed changes.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2019

Can we unblock this. It it seems readyish. Just need a refresh/rebase.

@bor0

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

Conflicts resolved, thanks!

@aduth aduth merged commit d45c8ea into WordPress:master Jul 29, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@github-actions github-actions bot added this to the Gutenberg 6.3 milestone Jul 29, 2019

@bor0 bor0 deleted the bor0:fix/avoid-unguarded-getrangeat branch Jul 29, 2019

gziolo added a commit that referenced this pull request Aug 29, 2019
Avoid usage of unguarded getRangeAt and add eslint rule (#16212)
* Avoid usage of unguarded getRangeAt and add eslint rule

* Address PR comments

* ESLint Plugin: Add CHANGELOG entry for avoid-unguarded-get-range-at

* ESLint Plugin: Rename avoid-unguarded-get-range-at to no-unguarded-get-range-at
gziolo added a commit that referenced this pull request Aug 29, 2019
Avoid usage of unguarded getRangeAt and add eslint rule (#16212)
* Avoid usage of unguarded getRangeAt and add eslint rule

* Address PR comments

* ESLint Plugin: Add CHANGELOG entry for avoid-unguarded-get-range-at

* ESLint Plugin: Rename avoid-unguarded-get-range-at to no-unguarded-get-range-at
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.