Skip to content

chore: convert some requires to import statements #112

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

Closed
wants to merge 1 commit into from

Conversation

aboyton
Copy link
Contributor

@aboyton aboyton commented Jan 21, 2019

This doesn't turn it on everywhere as doing so made some of the tests to fail.

This doesn't turn it on everywhere as doing so made some of the tests to fail.
@JamesHenry
Copy link
Member

JamesHenry commented Jan 21, 2019

Thanks for looking to contribute, @aboyton! I'm afraid I don't understand the goal of this PR, and it is fundamentally broken. You are adding imports and exports to node .js files, that will not work. Also "resolveJsonModule": true will alter the structure of the build output, so has significant repercussions.

These are not changes we can make right now but thanks again for supporting the project

@JamesHenry JamesHenry closed this Jan 21, 2019
@aboyton
Copy link
Contributor Author

aboyton commented Jan 22, 2019

All good. I was looking for an easy thing to get started on the project again and noticed that #67 had a comment

i had to disable some of recommended rules, and we should fix issues reported by them.

and so I figured I'd try see if I could turn some of them on. I wasn't sure if this would break anything or not, I ran the tests and then figured why not see if this was correct or useful. Sorry to waste your time.

@aboyton aboyton deleted the require-to-import branch January 22, 2019 00:05
@JamesHenry
Copy link
Member

JamesHenry commented Jan 22, 2019

Oh not at all, I really appreciate the offer of help! I'm afraid the new setup is just a little too new for us to have figured out good contributing guidance etc.

We'll definitely improve on that and hopefully we'll see some more PRs from you in the near future :)

@armano2
Copy link
Collaborator

armano2 commented Jan 22, 2019

idea of turning off @typescript-eslint/no-var-requires in js only files seems valid

@aboyton you can try updating it in ts files only and turning this rule off for all js files

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants