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

Rule proposal: Prefer String#startsWith() and String#endsWith() #285

Closed
sindresorhus opened this issue Feb 14, 2019 · 6 comments · Fixed by #289
Closed

Rule proposal: Prefer String#startsWith() and String#endsWith() #285

sindresorhus opened this issue Feb 14, 2019 · 6 comments · Fixed by #289
Labels
enhancement: new plugin rule New rule request for eslint-plugin package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@sindresorhus
Copy link

The rule would catch the following cases:

.startsWith instead of:

  • foo.indexOf('bar') === 0
  • foo[0] === 'b'
  • foo.charAt(0) === 'b' (and foo.charAt())
  • foo.slice(0, 2) === 'bar' (and .substring & .substr)
  • /^bar/.test(foo) (and .match())

.endsWith instead of:

  • .lastIndexOf
  • foo[foo.length - 1] === 'r'
  • foo.charAt(foo.length - 1) === 'b'
  • foo.slice(0, -3) === 'bar' (and .substring & .substr)
  • /bar$/.test(foo) (and .match())

The regex case is especially useful as I've seen a lot of regexes used in this scenario.

@sindresorhus sindresorhus added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels Feb 14, 2019
@mysticatea
Copy link
Member

I will work on this after accepted by the core team.

@bradzacher
Copy link
Member

I don't necessarily agree that
foo[0] === 'b' is equivalient to foo.startsWith('b') (and same for the endsWith example)
Purely because using startsWith for a single character will be significantly slower than using the the character indexer:
https://www.measurethat.net/Benchmarks/Show/3319/0/startswith-vs-charat-for-single-character
image

@bradzacher bradzacher added enhancement: new plugin rule New rule request for eslint-plugin and removed triage Waiting for maintainers to take a look labels Feb 15, 2019
@sindresorhus
Copy link
Author

@bradzacher I'm not convinced by a micro-benchmark. Whether this actually matters in production code is unclear. Also, startsWith is a fairly recent addition and has not had time to be properly optimized by V8 yet. Most devs use other newer JS features, like for-of instead of for even though for is still faster (in micro-benchmarks).

@bradzacher
Copy link
Member

I went and found that benchmark to confirm my thinking was correct.

charAt/index access should dive straight into the memory via pointer arithmetic - i.e. should be a near free, super cheap operation.
startsWith will require it to set up a new stack frame, initialise whatever internal variables, and start a loop (which iterates once).

The overhead of that clearly adds up!

You're right though that they might consider adding a special code path for starts/endsWith being called with a single character.

We would probably want to add some settings to allow people to customise what's checked.

@sindresorhus
Copy link
Author

You're right though that they might consider adding a special code path for starts/endsWith being called with a single character.

https://twitter.com/ryzokuken/status/1096281036403691520 :)

We would probably want to add some settings to allow people to customise what's checked.

Sure. I think foo[0]/foo.charAt(0) is really the only case that needs a setting.

@bradzacher
Copy link
Member

Haha! I didn't know you had a direct line to the V8 devs :P
That's sick though. If there looking to optimise it asap, then in that case I wouldn't even worry about a setting. We can just document it in the readme to head off any possible enhancement requests :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement: new plugin rule New rule request for eslint-plugin package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
3 participants