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

fix: Improve parse perf when using `@typescript-eslint/parser` #1409

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@bradzacher
Copy link
Contributor

commented Jul 8, 2019

Fixes #1408
See the above issue for justification.

fix: Improve parse perf when using `@typescript-eslint/parser`
Fixes #1408
See the above issue for justification.
@coveralls

This comment has been minimized.

Copy link

commented Jul 8, 2019

Coverage Status

Coverage remained the same at 97.753% when pulling eaf3c35 on bradzacher:patch-1 into 6512110 on benmosher:master.

Show resolved Hide resolved utils/parse.js
@ljharb

ljharb approved these changes Jul 8, 2019

Copy link
Collaborator

left a comment

This looks good, but rather than mutating the new parserOptions object, maybe we could change this entire block to:

parserOptions = Object.assign({}, parserOptions, {
  ecmaFeatures: Object.assign({}, parserOptions.ecmaFeatures),
  comment: true,
  attachComment: true,
  tokens: true,
  loc: true,
  range: true,
  filePath: path,
  project: undefined,
  projects: undefined,
})

?

@ljharb ljharb requested a review from benmosher Jul 8, 2019

@ljharb ljharb added the typescript label Jul 8, 2019

@ljharb

This comment has been minimized.

Copy link
Collaborator

commented Jul 9, 2019

LGTM; i'm not sure if appveyor is failing on master or only on this PR.

Revert "use object.assign more"
This reverts commit 659e557.
@bradzacher

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

@ljharb - appveyor was passing with my first commit - it appears using Object.assign breaks some tests.
It's a bit hard to investigate, because locally no matter what I do, there are many failing tests locally for some reason...
I'll just revert the commit.

@ljharb

This comment has been minimized.

Copy link
Collaborator

commented Jul 9, 2019

That's a shame. We can fix that in a followup PR.

@deser

deser approved these changes Jul 10, 2019

@benmosher

This comment has been minimized.

Copy link
Owner

commented Jul 12, 2019

No need to re-Object.assign, the object is already shallowly cloned on line 19. LGTM! thanks!

@lencioni
Copy link
Contributor

left a comment

Thanks for working on this! Excited to see this land.

@deser

This comment was marked as off-topic.

Copy link

commented Jul 12, 2019

Guys, do you have vision when it would be delivered\published?

@ljharb

This comment has been minimized.

Copy link
Collaborator

commented Jul 12, 2019

@benmosher it’s still better to create a new object all at once instead of mutating it at any point.

@benmosher

This comment has been minimized.

Copy link
Owner

commented Jul 12, 2019

fair enough, but even if we used destructuring to remove the fields (iff this file was being transpiled) the Babel'd version would start with an empty object and add fields.

given that we can't predict all the fields we need to pass through, this seems acceptable to me. I'm good to merge if you are, @ljharb.

@ljharb

This comment has been minimized.

Copy link
Collaborator

commented Jul 12, 2019

Definitely, we can look into creating the object differently in a follow up. Once tests are passing, it’s good to go.

@bradzacher

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2019

Once tests are passing, it’s good to go.

@ljharb - having a look, the travis CI tests that are failing are the same ones that are failing in master right now (tests against eslint v2/v3)

@ljharb

This comment has been minimized.

Copy link
Collaborator

commented Jul 12, 2019

We should fix those in master first, then, and rebase this PR

@deser

This comment was marked as off-topic.

Copy link

commented Jul 12, 2019

Or just merge and make all happy!)

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.