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

Fix a bug in layouts with props that use type variables #128

Merged

Conversation

Ferdev
Copy link
Contributor

@Ferdev Ferdev commented Jul 22, 2023

Problem

If the project has a layout with props using type variables, there's an error in the generated code that throws:

Props type needs 1 argument, but I see 0 instead: in .elm-land/src/Layouts.elm

Apparently, the implementation in the code generator for layouts that use props with type variables is incomplete. This PR tries to fill the missing gaps.

Solution

This commit updates the array of layout paths sent to the code generator, including a boolean variable indicating if the layout uses Props with type variables. In such case, the code generator creates valid code for that layout, avoiding the current error.

Notes

Issue mentioned and discussed in Discord

If the project has a layout with props using type variables, there's
an error in the code generation that throws:

```
Props type needs 1 argument, but I see 0 instead: in .elm-land/src/Layouts.elm
```

This commit updates the array of layout paths sent to the code
generator, including a boolean variable indicating if the layout uses Props
with type variables. In such case, the code generator creates valid
code for that layout, avoiding the current error.
@netlify
Copy link

netlify bot commented Jul 22, 2023

Deploy Preview for elm-land canceled.

Name Link
🔨 Latest commit 4644ffe
🔍 Latest deploy log https://app.netlify.com/sites/elm-land/deploys/64e84a8f99db4e00085fbe9a

@ryan-haskell ryan-haskell self-requested a review July 22, 2023 23:19
@mc706
Copy link

mc706 commented Aug 2, 2023

copying here from discord for visibility

Ryan (he/him) — 07/30/2023 5:25 PM
@samus the code looks great– would you mind adding a new example in the examples folder of the repo that uses the feature you fixed?
That would help prevent regressions and allow me to quickly test the before/after behavior when reviewing locally ❤️

@mc706
Copy link

mc706 commented Aug 12, 2023

Circling back, is there anything i can do to help move this along? really looking to try out Layouts

@Ferdev
Copy link
Contributor Author

Ferdev commented Aug 13, 2023

hey Ryan, I'm sorry, I haven't had much free time lately to wrap up this PR. I'll try to give it a big push during this weekend.

I'm having a problem making the cli to generate the code of the example. It will only add the default app that displays Hello, Elm Land!. So any hints to debug the problem greatly appreciated!

@ryan-haskell
Copy link
Contributor

hey Ryan, I'm sorry, I haven't had much free time lately to wrap up this PR.

No worries at all (or need to apologize)! You've already contributed more than I could have asked for ❤️


@mc706 , if you'd like to add a minimal example in the examples folder that captures the feature that isn't working as expected, that would be great.

You don't have to get it running locally, even a separate PR that just adds the broken example would be more than enough to get this merged with the next release!

Thank you both for your contributions so far– I truly appreciate it 💖

@Ferdev
Copy link
Contributor Author

Ferdev commented Aug 23, 2023

Hey @ryannhg! Finally got some time to add a very simple example (number 17, feel free to change the ordering).

I think we may have a bug with nested layouts. While I was working on the example, at some point I couldn't compile example 08 (nested layouts). Can you give it a try using the codegen-worker this PR generates?

Thanks!

@ryan-haskell
Copy link
Contributor

@Ferdev thank you!

As a heads up, I just enabled GitHub Actions for this PR, so you will be able to see if any tests are failing before/after your changes:

image

Hopefully, this will let you validate if there are any regressions as you go! 🪄

@Ferdev
Copy link
Contributor Author

Ferdev commented Aug 25, 2023

I think I've fixed the regression. The test that was failing is no passing in my machine. Although other tests pass/fail randomly in my system, so I'm not 100% sure if the whole test suite passes.

@ryan-haskell ryan-haskell added 🚀 Next release This will ship with the next Elm Land release Bug Something isn't working as promised labels Aug 30, 2023
@ryan-haskell ryan-haskell self-assigned this Aug 30, 2023
@ryan-haskell ryan-haskell merged commit e8f0fc3 into elm-land:main Aug 30, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working as promised 🚀 Next release This will ship with the next Elm Land release
Development

Successfully merging this pull request may close these issues.

None yet

3 participants