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

Build failures with 3.0.17 #501

Closed
ianmackenzie opened this issue Dec 2, 2024 · 15 comments · Fixed by #502
Closed

Build failures with 3.0.17 #501

ianmackenzie opened this issue Dec 2, 2024 · 15 comments · Fixed by #502

Comments

@ianmackenzie
Copy link

We just tried updating from 3.0.16 to 3.0.17 at work, but started getting a bunch of Elm compiler errors when running elm-pages build:

image

Is it possible that the update to elm-codegen v5 broke code generation in some cases, or is this likely a symptom of something else? (The error happens both locally and in CI, including after deleting .elm-pages, elm-stuff and node_modules, so I don't think it's a corrupt build cache or anything like that.)

(And yes, as you might be able to guess from the paths we are indeed using elm-pages for our public web site! Thanks for all your work on it! 🙏)

@dillonkearns
Copy link
Owner

Hello @ianmackenzie, thank you for opening the issue! I actually just saw this as well when I tried to upgrade the starter repo. I was thinking the same thing that it is likely related to the elm-codegen upgrade (#488), since that's the only thing that would be related to changing that generated code since the 3.0.17. @miniBill do you have any thoughts on that? The only things I can think of are:

  1. The updated elm-codegen Elm package
  2. The updated elm-codegen NPM package
  3. The code changes made during the migration
  4. A combination of the above

@dillonkearns
Copy link
Owner

Also, wonderful to hear that you're building your site with elm-pages! 😄

@miniBill
Copy link
Contributor

miniBill commented Dec 2, 2024

I actually get the same error 😅
I "fixed" it by creating empty modules with those names, but didn't have time to fully investigate.

I think it may be a bug in elm-codegen, will need to dive into it!

@miniBill
Copy link
Contributor

miniBill commented Dec 2, 2024

@ianmackenzie if you create Pages/ProgramConfig.elm and Internal/ApiRoute.elm the project will compile.

@dillonkearns
Copy link
Owner

Interesting, thanks for the quick response @miniBill! That is good context and does narrow things down. I was looking through GenerateMain.elm and I didn't see any changes in there that seemed like they could be connected to new issues there, so that would also support that theory that there is a bug or change somewhere in elm-codegen.

@miniBill
Copy link
Contributor

miniBill commented Dec 2, 2024

succeed: a -> Internal.ApiRoute.ApiRouteBuilder a (List String)
-}
succeed : Elm.Expression -> Elm.Expression
succeed succeedArg_ =
    Elm.apply
        (Elm.value
             { importFrom = [ "ApiRoute" ]
             , name = "succeed"
             , annotation =
                 Just
                     (Type.function
                          [ Type.var "a" ]
                          (Type.namedWith
                               [ "Internal", "ApiRoute" ]
                               "ApiRouteBuilder"
                               [ Type.var "a", Type.list Type.string ]
                          )
                     )
             }
        )
        [ succeedArg_ ]

I think this is the culprit

@miniBill
Copy link
Contributor

miniBill commented Dec 2, 2024

Yeah, I think that this might be elm-codegen picking Internal.ApiRoute.ApiRouteBuilder instead of the local alias type alias ApiRouteBuilder a constructor

@miniBill
Copy link
Contributor

miniBill commented Dec 2, 2024

It may be possible to work around this in elm-pages by using withType on those functions, but I'd rather fix it upstream

@miniBill
Copy link
Contributor

miniBill commented Dec 2, 2024

Ok, I think I have a clean workaround: don't import ApiRouteBuilder from Internal.ApiRoute but use it qualified. In that case elm-codegen will generate the correct binding.

Same for ProgramConfig.

@dillonkearns do you want a PR?

@miniBill
Copy link
Contributor

miniBill commented Dec 2, 2024

(or wait for the fix to mdgriffith/elm-codegen#104 to drop, that also works)

@dillonkearns
Copy link
Owner

If you’re up for it, I think a PR with the workaround would be excellent @miniBill! It would be good to get something out to get the release fixed to avoid confusion for anyone who installs it

@miniBill
Copy link
Contributor

miniBill commented Dec 3, 2024

Ok, I think #502 works as a fix. I quickly tested it and it seems to not generate the spurious imports anymore.

@dillonkearns
Copy link
Owner

This is working now with the help of #503. Thank you so much for investigating and fixing the issue @miniBill, and for the quick turnaround! 🙏

NPM version 3.0.19 is working now. Let me know how it goes for you @ianmackenzie!

@ianmackenzie
Copy link
Author

3.0.19 seems to work great, thanks for the quick work @miniBill and @dillonkearns!

@dillonkearns
Copy link
Owner

Excellent, thank you for confirming @ianmackenzie!

I wonder if we'll be seeing faster build times as a result of the upgraded dependencies here, too, I would imagine so.

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 a pull request may close this issue.

3 participants