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

[TypeScript] Phase 1 #3267

Closed
wants to merge 8 commits into from
Closed

Conversation

dimitropoulos
Copy link
Contributor

This ticket implements Phase 1 of the TypeScript conversion: #3266

The following PRs were "pre-reviews" of all of the work contained in this PR.

  1. insomnia-xpath [TypeScript] insomnia-xpath dimitropoulos/insomnia#1
  2. insomnia-url [TypeScript] insomnia-url dimitropoulos/insomnia#2
  3. insomnia-prettify [TypeScript] insomnia-prettify dimitropoulos/insomnia#3
  4. insomnia-importers [TypeScript] insomnia-importers dimitropoulos/insomnia#4
  5. insomnia-cookies [TypeScript] insomnia-cookies dimitropoulos/insomnia#5
  6. insomina-testing [TypeScript] insomnia-testing dimitropoulos/insomnia#6
  7. insomnia-inso [TypeScript] insomnia-inso dimitropoulos/insomnia#7

Comment on lines +26 to +27
// @ts-expect-error something is wrong here, ultimately these types come from https://nodejs.org/api/util.html#util_util_inspect_object_options and `date` doesn't appear to be one of the options.
date: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -9,13 +9,13 @@
},
"homepage": "https://github.com/kong/insomnia#readme",
"scripts": {
"lint": "eslint . --ext .js,.json",
"lint": "lerna run lint --parallel --stream",
Copy link
Contributor

@develohpanda develohpanda Apr 12, 2021

Choose a reason for hiding this comment

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

I understand that linting in parallel might be faster but duplicating the lint script across all packages seems unnecessary/impractical when it comes to making a change and having to remember to update all lint scripts to keep them aligned.

I'm curious to understand your thought process behind making this change (if it's documented somewhere please point me to it). Is it so that each package can operate as a single/standalone entity?

Copy link
Contributor Author

@dimitropoulos dimitropoulos Apr 12, 2021

Choose a reason for hiding this comment

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

I did this because it seemed like an error to be honest. it's a quite significant amount of time, and, afterall, the thing that ultimately runs this is the CI. It was changed while splitting the typechecking step out on the CI. More than happy to consider it out of scope and change it back if you'd like - lemme know.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just thinking out loud after our verbal convo - how do other lerna projects handle this? A single eslint call at the root, or individual ones in each package?

If we have eslintignore in each package now, I think we should be able to remove it from the root altogether because IDEs should correctly load from the appropriate package (right?)

Copy link
Contributor Author

@dimitropoulos dimitropoulos Apr 13, 2021

Choose a reason for hiding this comment

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

predominantly (from my looking-around during Phase 0): they just use yarn. EDIT: I misunderstood your question. They predominantly do something similar to what we had before, but it doesn't work for us right now from the root because different packages use different parsers and you can only have one parser per running eslint instance. One way or the other, this is fairly optimal for us because it can do it multi-threaded this way so there's no reason I can see to remove it.

re: .eslintignore at the root: we can drastically reduce it, yes, but it will likely need to stay since, after all, there are still files at the root (including the root eslint config itself) that eslint checks.

Copy link
Contributor

@develohpanda develohpanda Apr 14, 2021

Choose a reason for hiding this comment

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

different packages use different parsers

I'm not sure I follow here 🤔 Wouldn't they all use @typescript-eslint/parser (at phase 2/3)?

One way or the other, this is fairly optimal for us because it can do it multi-threaded this way

For performance, yes I agree but that's at the cost of misdirection where .eslintrc.js is defined at the root, but .eslintignore is defined per package 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They will use the same parser by phase 2, but this is only a PR for phase 1.

Re: .eslintignore I'm not sure I see the downside of having individual .eslintignores wherever we need them. This keeps the packages more isolated from each other, which is ideal in a monorepo because it means that there's that much less you have to know about the other packages in order to examine a specific one.

package.json Outdated Show resolved Hide resolved
"package.json"

],
"exclude": []
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to explicitly exclude bin? When I run build insomnia-importers, the bin/cli file gets updated

@dimitropoulos
Copy link
Contributor Author

closing this PR, as #3370 contains this PR's code as well as (almost all of) Phase 2's work.

@dimitropoulos dimitropoulos deleted the typescript branch May 11, 2021 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants