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

Remove pages from frontend bundle #478

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MartinSStewart
Copy link

These changes should prevent code in the pages field from getting bundled with the frontend

Copy link

netlify bot commented May 28, 2024

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

Visit the deploys page to approve it

Name Link
🔨 Latest commit 6cdb1c8

@dillonkearns
Copy link
Owner

Thanks for the PR! It's close, getting this failure in the CI:

-- TYPE MISMATCH --------------- /home/runner/work/elm-pages/elm-pages/examples/routing/elm-stuff/elm-pages/client/app/Route/Date/SPLAT_.elm
The 1st argument to `preRender` is not what I expect:

31|     RouteBuilder.preRender
32|>        { head = head
33|>        , pages = \_ -> BackendTask.fail (FatalError.fromString "")
34|>        , data = \_ -> BackendTask.fail (FatalError.fromString "")
35|>        }

This argument is a record of type:

    { data : RouteParams -> BackendTask FatalError Data
    , head : App Data ActionData RouteParams -> List Head.Tag
    , pages : b -> BackendTask FatalError a
    }

But `preRender` needs the 1st argument to be:

    { data : RouteParams -> BackendTask FatalError Data
    , head : App Data ActionData RouteParams -> List Head.Tag
    , pages : BackendTask FatalError (List RouteParams)
    }

So it shouldn't have a lambda parameter for the case of the pages record field.

Also, I'd like to have a test case to reflect this in https://github.com/dillonkearns/elm-pages/blob/6cdb1c8465e7cd035736d2a150be77226334e62f/generator/dead-code-review/tests/Pages/Review/DeadCodeEliminateDataTest.elm before merging and shipping this.

@MartinSStewart
Copy link
Author

Sorry about the delayed response. I ran into issues when trying to run the elm-pages tests (Windows line-endings causing trouble) so I switched to my mac and then ran into NPM issues there. I've lost motivation to solve this so it's probably best to just close this PR.

@dillonkearns
Copy link
Owner

Hey @MartinSStewart, thanks for letting me know!

I'll leave this open for a bit in case anybody wants to try to pick it up. Would be a great one for someone to try to contribute I think, and this would be a very nice improvement. Thanks for getting the ball rolling on this! I'll close it if it's stale after a while.

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