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

Typing Updates #166

Merged
merged 8 commits into from
Jun 22, 2024
Merged

Typing Updates #166

merged 8 commits into from
Jun 22, 2024

Conversation

lishaduck
Copy link
Contributor

This doesn't touch implementations, thus the branch name.

Depends on #165

Here's the diff as GH doesn't play well with stacks from forks: lishaduck/node-elm-review@jsdocs...pure-typings

Copy link

socket-security bot commented Jun 22, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@types/fs-extra@9.0.13 None 0 27.9 kB types
npm/@types/prompts@2.4.9 None +1 17.9 kB types
npm/@types/which@2.0.2 None 0 6.06 kB types
npm/@types/wrap-ansi@8.0.2 None 0 4.27 kB types

🚮 Removed packages: npm/npm-run-all@4.1.5

View full report↗︎

@lishaduck lishaduck force-pushed the pure-typings branch 2 times, most recently from 9b5473c to aac34a9 Compare June 22, 2024 02:31
@@ -14,7 +14,6 @@
"prettier-check": "prettier . --check",
"prettier-fix": "prettier . --write",
"tsc": "tsc && tsc --project tsconfig.no-implicit-any.json",
"tsc-watch": "tsc --watch",
Copy link
Owner

Choose a reason for hiding this comment

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

How can we run tsc in watch mode using turbo? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

turbo watch tsc
It'll cache a bit more, and isn't any more characters.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm getting this error when trying it:

• Packages in scope: 
• Running tsc in 0 packages
• Remote caching disabled
  × failed to connect to daemon
  ╰─▶ server is unavailable: channel closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. Ok, I'll revert it.

@lishaduck
Copy link
Contributor Author

Ah, silly me. I pushed the fix to my next branch. I'll cherry-pick it soonish, and this should be good to go.

@jfmengels
Copy link
Owner

Alright.
Note that I just merged a PR by someone else (#162, the final commit in main being 7ba6bd7), which may conflict with this branch. If I'm not mistaken, it's only the rename from ruleType to _ruleType that need to be skipped.

@lishaduck
Copy link
Contributor Author

Alright. Note that I just merged a PR by someone else (#162, the final commit in main being 7ba6bd7), which may conflict with this branch. If I'm not mistaken, it's only the rename from ruleType to _ruleType that need to be skipped.

Thanks for letting me know!

@jfmengels
Copy link
Owner

CI is failing:

Run turbo run test
• Running test
• Remote caching disabled
x root task //#test (turbo testing) looks like it invokes turbo and might
| cause a loop

I think I should have updated CI to run either npm test or turbo testing instead of turbo testing. Does that sound correct to you?

@lishaduck
Copy link
Contributor Author

CI is failing:

Run turbo run test
• Running test
• Remote caching disabled
x root task //#test (turbo testing) looks like it invokes turbo and might
| cause a loop

I think I should have updated CI to run either npm test or turbo testing instead of turbo testing. Does that sound correct to you?

Yeah. I fixed it :)

@lishaduck
Copy link
Contributor Author

Yeah. I fixed it :)

I take that back. What's up with haste-map? It looks like jest is failing now.

@jfmengels
Copy link
Owner

Could it be that turbo / CI is caching results from previous runs and that that causes problems?

@lishaduck
Copy link
Contributor Author

lishaduck commented Jun 22, 2024

Could it be that turbo / CI is caching results from previous runs and that that causes problems?

I don't think so:

cache bypass, force executing 2d4e737178c4d703

So it's not using the cache. Does jest need to run before test-run? I did parallelize it, which might be the issue.

@lishaduck
Copy link
Contributor Author

Could it be that turbo / CI is caching results from previous runs and that that causes problems?

I don't think so:

cache bypass, force executing 2d4e737178c4d703

So it's not using the cache. Does jest need to run before test-run? I did parallelize it, which might be the issue.

Hmmm. Locally, test-run works in parallel, which is good. I must've just corrupted it by quitting npm at a bad time. jest is crashing in elm-tooling.test. I'll look into it. After all, this shouldn't change impls. That's the point of this branch.

@lishaduck
Copy link
Contributor Author

lishaduck commented Jun 22, 2024

Oh, could it be --detectOpenHandles? I added that to catch some regressions I made, maybe there were some other ones?

Could it be that turbo / CI is caching results from previous runs and that that causes problems?

I don't think so:

cache bypass, force executing 2d4e737178c4d703

So it's not using the cache. Does jest need to run before test-run? I did parallelize it, which might be the issue.

Also, it's reproducible with just npm run jest.
Does jest redirect output? console.log at the top of elm-tooling.test.js never logs. It does 😞

@lishaduck
Copy link
Contributor Author

I figured it out. Somehow, elm-review is parsing jest flags as it's own, but then the log gets overriden by jest so you can't read it.
It's probably a bug, but it can go into #156 for now.

@lishaduck lishaduck mentioned this pull request Jun 22, 2024
@jfmengels jfmengels merged commit 858533f into jfmengels:main Jun 22, 2024
3 checks passed
@jfmengels
Copy link
Owner

Thank you for looking into all of this! 🙏

@lishaduck lishaduck deleted the pure-typings branch June 22, 2024 16:05
@lishaduck lishaduck mentioned this pull request Jun 28, 2024
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