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

Miscellaneous Internal Changes #499

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

lishaduck
Copy link

@lishaduck lishaduck commented Nov 27, 2024

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

  1. I decided to try jj out today. It's nice, but doesn't support https auth.

Copy link

netlify bot commented Nov 27, 2024

👷 Deploy request for elm-pages-todos pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 7c832d1

@lishaduck lishaduck changed the title Vite6 Misc Changes Nov 27, 2024
@lishaduck
Copy link
Author

Hey, look at that! I force pushed :)
I feel really silly relearning a vcs, but it's a fun break from normalcy for thanksgiving

Copy link
Author

@lishaduck lishaduck left a 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,
Copy link
Author

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"]
Copy link
Author

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.",
Copy link
Author

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,
Copy link
Author

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",
Copy link
Author

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.

Copy link
Author

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",
Copy link
Author

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",
Copy link
Author

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.

Copy link
Author

@lishaduck lishaduck left a 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"],
Copy link
Author

@lishaduck lishaduck Nov 27, 2024

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.

Copy link
Author

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?

jsconfig.json Outdated
Copy link
Author

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
Copy link
Author

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.

@lishaduck lishaduck changed the title Misc Changes Miscellaneous Internal Changes Nov 28, 2024
@lishaduck
Copy link
Author

@dillonkearns, ping :)

@miniBill
Copy link
Contributor

miniBill commented Dec 5, 2024

Could you minimize the diff? There is some formatting noise.
In general these all seem reasonable, but I haven't inspected them deeply.

@lishaduck
Copy link
Author

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).
I'm happy to revert it, just wanted to explain my rationale for not doing so.

@lishaduck
Copy link
Author

lishaduck commented Dec 5, 2024

Oh, I don't have enough context to be sure, but it looks like request-cache-fs.js unused, and if it's deleted, you can also remove the memfs dependency.

Maybe I'll knip and report back.

EDIT: Here's results (I removed the most obvious false-positives):

Unused Summary

Unused files (82)
examples/**
generator/src/render-test.js
generator/src/render-worker.js
generator/src/request-cache-fs.js
generator/static-code/elm-pages.js
generator/static-code/hmr.js
generator/template/custom-backend-task.ts
generator/template/elm-pages.config.mjs
generator/template/index.ts
generator/template/script/.elm-pages/compiled-ports/custom-backend-task.mjs
generator/template/script/custom-backend-task.ts
Unused dependencies (1)
jsesc  package.json
Unused devDependencies (5)
@types/jsesc          package.json
elm-test              package.json
Unlisted binaries (2)
elm-pages  .github/workflows/ci.yml
elm-pages  package.json
Unresolved imports (1)
../generator/src/generate-records.js  tests/generated-files.test.js:3:33
Unused exports (24)
elmOptimizeLevel2       build           function  generator/src/build.js:453:17                
writeFileSyncSafe       fs              function  generator/src/dir-helpers.js:38:17           
copyDirFlat             fs              function  generator/src/dir-helpers.js:47:23           
copyDirNested           fs              function  generator/src/dir-helpers.js:61:23           
restoreColor                            unknown   generator/src/error-formatter.js:84:14       
replaceTemplate         preRenderHtml   function  generator/src/pre-render-html.js:71:17       
pipeShells              renderer        function  generator/src/render.js:1016:17              
question                renderer        function  generator/src/render.js:1101:23              
render                  renderer        function  generator/src/render.js:45:23                
shell                   renderer        function  generator/src/render.js:951:17               
routeVariantDefinition  routeHelpers    function  generator/src/route-codegen-helpers.js:126:17
joinPath                routeHelpers    function  generator/src/route-codegen-helpers.js:202:17
parseRouteParams        routeHelpers    function  generator/src/route-codegen-helpers.js:20:17 
newHelper               routeHelpers    function  generator/src/route-codegen-helpers.js:214:17
toElmPathPattern        routeHelpers    function  generator/src/route-codegen-helpers.js:241:17
camelToKebab            routeHelpers    function  generator/src/route-codegen-helpers.js:300:17
paramsRecord            routeHelpers    function  generator/src/route-codegen-helpers.js:307:17
routeVariant            routeHelpers    function  generator/src/route-codegen-helpers.js:329:17
destructureRoute        routeHelpers    function  generator/src/route-codegen-helpers.js:337:17
referenceRouteParams    routeHelpers    function  generator/src/route-codegen-helpers.js:343:17
emptyRouteParams        routeHelpers    function  generator/src/route-codegen-helpers.js:349:17
toFieldName             routeHelpers    function  generator/src/route-codegen-helpers.js:356:17
routeParams             routeHelpers    function  generator/src/route-codegen-helpers.js:4:17  
isReadableStream        validateStream  function  generator/src/validate-stream.js:3:17

@miniBill
Copy link
Contributor

miniBill commented Dec 5, 2024

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). I'm happy to revert it, just wanted to explain my rationale for not doing so.

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?

@lishaduck
Copy link
Author

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.

@dillonkearns
Copy link
Owner

Yes, I'd be very happy to accept a formatting PR!

In general, I'm finding it hard to review this PR because there is a lot going on, and in particular a lot of things which I have trouble verifying.

If it's something that has a unit test and I can see it passing, then I can confidently review and merge. There are some things in here which won't really have an impact on user-facing things, so they're more low-risk. But there is a lot going on that is hard to separate out. If you're up for it, I would find it a lot easier to split this up into a few smaller PRs that target related areas. Ideally, it would be nice if I could review some PRs where it's very clear there is no risk or low risk for user-facing issues and separate that out from things that need more careful verification.

Happy to discuss that strategy ore if you would like!

@lishaduck
Copy link
Author

lishaduck commented Dec 28, 2024

Happy to discuss that strategy ore if you would like!

I guess a treewide format would be one, updating typescript stuff would be another (fixes to types, tsconfig settings, and @types packages, in separate commits?), deleting dead code is another, and updating the package description would be the other one.

Do those four sound good, or would you prefer if I also split up the ts ones as well?

@dillonkearns
Copy link
Owner

That sounds excellent, yes! That would be a lot easier for me to go through quickly and separate out what is low risk 👍

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.

3 participants