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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

ts-migration/index #372

Merged
merged 8 commits into from Aug 6, 2019
Merged

ts-migration/index #372

merged 8 commits into from Aug 6, 2019

Conversation

G-Rath
Copy link
Collaborator

@G-Rath G-Rath commented Aug 6, 2019

chore(index): convert to typescript.

Might as well break this out too 馃槀

@G-Rath G-Rath requested a review from SimenB August 6, 2019 09:44
src/index.ts Outdated Show resolved Hide resolved
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

see inline comment

@SimenB
Copy link
Member

SimenB commented Aug 6, 2019

This fails locally due to #371 not being merged. Can land after that's done 馃檪

@G-Rath
Copy link
Collaborator Author

G-Rath commented Aug 6, 2019

haha yeah I saw that - I'm doing that right now.

btw do you want parse in this?

@SimenB
Copy link
Member

SimenB commented Aug 6, 2019

Ah yeah, parse looks like a nice improvement over basename

@G-Rath
Copy link
Collaborator Author

G-Rath commented Aug 6, 2019

.filter(rule => !excludedFiles.includes(rule))

Woops... 馃槄

I was about to amend the "bad" commit message, but now that you've pushed, I'll leave it to you if that's ok.

Once this has been merged would you mind rebasing #363 for me? I did it locally, but something weird happened and it decided to not apply the change to snapshot-processor.test.ts so it was using the old const { snapshotProcessor } import 馃槵

I can do it if you're not got the time :)


return rules;
},
(rules, key) => ({ ...rules, [`jest/${key}`]: 'error' }),
Copy link
Collaborator Author

@G-Rath G-Rath Aug 6, 2019

Choose a reason for hiding this comment

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

This is actually surprisingly "slow" vs using an assignment, b/c everytime you're creating a new object & copying all the keys.

It's by far not a big deal, and even less so since we're doing it the once (I hope? I assume the way eslint runs it typically reuses the same node process), but it's an interesting little fact.

(That might have changed in later versions of node - iirc I last tested this on node8)

Copy link
Member

Choose a reason for hiding this comment

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

doing it once during require, yeah - probably not even measurable except in micro benchmarks. Nothing compared to the FS operations in the same file 馃檪

Copy link
Member

Choose a reason for hiding this comment

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

and yeah, object spread has gotten way quicker (although it's currently transpiled down by babel until we drop node 6)

https://twitter.com/bmeurer/status/1015105293301280768?lang=en

@SimenB SimenB merged commit a334368 into master Aug 6, 2019
@SimenB SimenB deleted the ts-migration/index branch August 6, 2019 20:06
@SimenB
Copy link
Member

SimenB commented Aug 6, 2019

Rebased #363 now. I think I did it right 馃槢

@SimenB
Copy link
Member

SimenB commented Aug 7, 2019

馃帀 This PR is included in version 22.15.0 馃帀

The release is available on:

Your semantic-release bot 馃摝馃殌

@SimenB SimenB added the released label Aug 7, 2019
@G-Rath
Copy link
Collaborator Author

G-Rath commented Aug 8, 2019

@SimenB

SyntaxError: /c/Users/G-Rath/workspace/projects-personal/eslint-plugin-jest/src/index.ts: export = is not supported by @babel/plugin-transform-typescript
Please consider using export <value>;.
31 |
32 | // eslint-disable-next-line import/no-commonjs
> 33 | export = {

That's the syntax for modules.export 馃槶

@SimenB
Copy link
Member

SimenB commented Aug 8, 2019

@G-Rath
Copy link
Collaborator Author

G-Rath commented Aug 8, 2019

oohhhohhh exciting!

My company does investment time (but are not thoughtbot 馃槈) every Friday (which is tomorrow), so I'll explore that plugin, and look about adding it at some point :D

@SimenB
Copy link
Member

SimenB commented Aug 8, 2019

Feel free to rip it out, publish it, and send a PR to jest migrating to using the published version 馃檪

@G-Rath
Copy link
Collaborator Author

G-Rath commented Aug 8, 2019

Sure thing :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants