Skip to content

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

Merged
ljharb merged 1 commit into
import-js:masterfrom
bradzacher:patch-1
Jul 19, 2019
Merged

fix: Improve parse perf when using @typescript-eslint/parser#1409
ljharb merged 1 commit into
import-js:masterfrom
bradzacher:patch-1

Conversation

@bradzacher

Copy link
Copy Markdown
Contributor

Fixes #1408
See the above issue for justification.

Fixes #1408
See the above issue for justification.
@coveralls

coveralls commented Jul 8, 2019

Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 97.896% when pulling 32704da on bradzacher:patch-1 into 3b21de6 on benmosher:master.

Comment thread utils/parse.js

@ljharb ljharb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 July 8, 2019 22:09
@ljharb

ljharb commented Jul 9, 2019

Copy link
Copy Markdown
Member

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

@bradzacher

Copy link
Copy Markdown
Contributor Author

@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

ljharb commented Jul 9, 2019

Copy link
Copy Markdown
Member

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

@benmosher

Copy link
Copy Markdown
Member

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

@lencioni lencioni left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@deser

This comment has been minimized.

@ljharb

ljharb commented Jul 12, 2019

Copy link
Copy Markdown
Member

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

@benmosher

Copy link
Copy Markdown
Member

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

ljharb commented Jul 12, 2019

Copy link
Copy Markdown
Member

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

@bradzacher

Copy link
Copy Markdown
Contributor Author

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

ljharb commented Jul 12, 2019

Copy link
Copy Markdown
Member

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

@deser

This comment has been minimized.

@deser

deser commented Jul 16, 2019

Copy link
Copy Markdown

If tests are failing in master can we just merge this and release?

@ljharb

ljharb commented Jul 16, 2019

Copy link
Copy Markdown
Member

No, we shouldn’t merge and must not release without passing tests.

@deser

deser commented Jul 16, 2019

Copy link
Copy Markdown

ok. sorry

@benmosher

Copy link
Copy Markdown
Member

sorry @deser, test failures were on me but they are fixed now 👍🏼

@bradzacher

bradzacher commented Jul 17, 2019

Copy link
Copy Markdown
Contributor Author

AppVeyor failed during install time on node8.
It looks like a transient error, but I don't have permission to retry the build.
Could someone with permissions please retry that build?

[00:00:33] npm ERR! code Z_BUF_ERROR
[00:00:33] npm ERR! errno -5
[00:00:33] npm ERR! zlib: unexpected end of file

If not I can just push a fluff commit to make it retry

@ljharb
ljharb merged commit 32704da into import-js:master Jul 19, 2019
@hayes

hayes commented Jul 19, 2019

Copy link
Copy Markdown

thanks for getting this merged!

@ljharb can you cut a release of eslint-module-utils as well? it looks like only eslint-plugin-import got released after this change

@bradzacher
bradzacher deleted the patch-1 branch July 19, 2019 17:01
@ljharb

ljharb commented Jul 19, 2019

Copy link
Copy Markdown
Member

Oops, yes will do today.

@ljharb

ljharb commented Jul 19, 2019

Copy link
Copy Markdown
Member

v2.4.1 of eslint-module-utils is published.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

Slowness when using typescript parser with project option

7 participants