-
-
Notifications
You must be signed in to change notification settings - Fork 97
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
Miscellaneous Internal Changes #499
base: master
Are you sure you want to change the base?
Conversation
👷 Deploy request for elm-pages-todos pending review.Visit the deploys page to approve it
|
Hey, look at that! I force pushed :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aaaaaaaand I found some more random stuff while perusing, but I'm awaiting an npm install
to verify some stuff.
@@ -4,9 +4,10 @@ | |||
"moduleResolution": "node", | |||
"checkJs": true, | |||
"allowJs": true, | |||
"noEmit": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise, I get an error about it can't output .js files because they're already authored in js.
"strict": true, | ||
"resolveJsonModule": true, | ||
"lib": ["es2015", "es2017.object", "esnext", "dom"] | ||
"lib": ["esnext", "dom"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was getting an error about bad targets & stuff, and this seemed to resolve it. The es*
libs implicitly include all the previous versions.
@@ -4,7 +4,7 @@ | |||
"version": "3.0.17", | |||
"homepage": "https://elm-pages.com", | |||
"moduleResolution": "node", | |||
"description": "Type-safe static sites, written in pure elm with your own custom elm-markup syntax.", | |||
"description": "Hybrid Elm framework with full-stack and static routes.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just took it from the repo description.
"strict": true, | ||
"resolveJsonModule": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It didn't look like this was needed anymore, and latest ts5.7 makes backcompat a pain, so I removed it.
@@ -1,12 +1,13 @@ | |||
{ | |||
"compilerOptions": { | |||
"types": ["node"], | |||
"moduleResolution": "node", | |||
"module": "NodeNext", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The node
value is deprecated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ⌘+F'd it and didn't see any references to this, so I took it to be unused.
@@ -3,7 +3,6 @@ | |||
"type": "module", | |||
"version": "3.0.17", | |||
"homepage": "https://elm-pages.com", | |||
"moduleResolution": "node", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't a thing.
"@types/make-fetch-happen": "^10.0.4", | ||
"@types/micromatch": "^4.0.9", | ||
"@types/node": "^22.10.0", | ||
"@types/serve-static": "^1.15.7", | ||
"@types/which": "~3.0.4", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TS was complaining about the lack of types for which, so I ran typesync.
Note that it uses a tilde, as DefinitelyTyped keeps majors and minors in sync with the upstream. Now, there aren't super recent types for which
, so it's a bit pointless, but it just did it automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last batch :)
@@ -1,12 +1,12 @@ | |||
{ | |||
"compilerOptions": { | |||
"types": ["node"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This prevented @types
packages from working.
generator/src/hello.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is unused?
The tsconfig overrides it.
jsconfig.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you've got both, most things won't work right.
@@ -3,6 +3,7 @@ | |||
"strict": true, | |||
"target": "ESNext", | |||
"module": "NodeNext", | |||
"moduleResolution": "NodeNext" | |||
"moduleResolution": "NodeNext", | |||
"checkJs": true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed that you want ts checking of js files, given the jsconfig.
@dillonkearns, ping :) |
Could you minimize the diff? There is some formatting noise. |
I could, but the repo has a prettier config, so I assumed it was oversight. GitHub only highlights non-whitespace changes, so all of the spots that got actual changes are both distinct and separate (the formatting didn't wasn't in the seconds I was editing). |
Oh, I don't have enough context to be sure, but it looks like Maybe I'll knip and report back. EDIT: Here's results (I removed the most obvious false-positives): Unused Summary
|
Makes sense! As an option, you could create a separate "format everything" PR and base this one on that one? @dillonkearns how do you feel about the idea? |
Or the other way around, you can't make stacked PRs from forks so there'd still be the diff. I'd be happy to do that if @dillonkearns approves. |
My attempt at #497. :)
I spent a few hours figuring out ssh auth,1 so you beat me to the push.
However, I caught a few other things, so I'm pushing anyway,
and will clean it up in a minute.Footnotes
I decided to try jj out today. It's nice, but doesn't support https auth. ↩