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 the options page in Elm #160

Closed
wants to merge 18 commits into from
Closed

Conversation

grahamc
Copy link
Member

@grahamc grahamc commented Sep 5, 2017

Just a last minute PR before vacation. Incorporates Elm in to the build phase, including pinned elm dependencies. PR version of http://gsc.io/nixos/nixos/options.html

I'll be back in a week if you have questions :)

@grahamc
Copy link
Member Author

grahamc commented Sep 5, 2017 via email

@zraexy
Copy link
Member

zraexy commented Sep 5, 2017

Looks good. The only issue I see is that it introduces a couple of regressions:

  • Every time a character is typed in the search box a new entry is pushed to history.
  • The back/forward buttons don't update the page.

It might also be good to always keep the query in the URL even when an option is selected so that if the page is refreshed the query is not lost

@grahamc
Copy link
Member Author

grahamc commented Sep 5, 2017

The back/forward buttons don't update the page.

Good catch!

Every time a character is typed in the search box a new entry is pushed to history.

Not sure how to fix this with Elm, but I'll research when I get back.

It might also be good to always keep the query in the URL even when an option is selected so that if the page is refreshed the query is not lost

This was a tough choice, but I think I settled on the right implementation: what if the query no longer matches the selected option when they come to it? Same for page number: what if it is on a different page? Once selected, IMO the query is not significant, just the selected option. Also, related options show up for free due to the alpha sorting.

@grahamc
Copy link
Member Author

grahamc commented Sep 13, 2017

@zraexy can you test again? http://gsc.io/nixos/nixos/options.html code needs cleaning up, but should work ok now w.r.t. back buttons and history pollution.

@grahamc
Copy link
Member Author

grahamc commented Sep 13, 2017 via email

nix_list list

GDict dict ->
if ((Dict.member "_type" dict) && ((Dict.get "_type" dict) == Just (GString "literalExample")) && (Dict.member "text" dict)) then
Copy link
Member

@prikhi prikhi Sep 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could turn this from an if/then to a case/of:

case (Dict.get "_type" dict, Dict.get "text" dict) of
  (Just (GString "literalExample"), Just (GString txt) ->
    txt
  (Just (GString "literalExample"), _ ) -> 
    "Literal example failed to product text"
  _ ->
     elseBranchCodeHere

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thank you!

entries =
Dict.toList dict
in
if List.length entries == 0 then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if List.isEmpty entries then

[ [ "{\n" ]
, indent
(List.concat
(List.map nix_enc_dict entries)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

List.concat ( List.map ... can be replaced with List.concatMap


nix_list : List Gson -> String
nix_list list =
if List.length list == 0 then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

List.isEmpty

Copy link

@zwilias zwilias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest running with elm-format@exp: it will remove unnecessary parens and reorder/cleanup imports. Especially the former may be of interest, there are a fair few redundant ones currently :)

@@ -0,0 +1,3 @@
elm-stuff/*
!elm-stuff/exact-dependencies.json
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not recommended. Couple of things to keep in mind:

  • this file is used to keep track of the entire dependency tree including transitive dependencies
  • elm does semver (and does it well)
  • if you require version pinning, the recommended approach is by selecting a specific version in elm-package.json by restricting the range (i.e. 1.1.1 <= v < 1.1.2 to pin to 1.1.1)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the reason I do this, is so Nix can do a pure build of the application. I'm parsing this file to pre-fetch dependencies at build time. See https://github.com/NixOS/nixos-homepage/pull/160/files#diff-42fbbaa4d2f9ce0b8c95980d6d43cc96R5 to about line 55.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Seems weird to bypass elm-package that way, but I don't know how Nix builds work or what the constraints are. I guess what I'm asking is this: what's the added value of this over elm-package install --yes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A build under Nix isn't allowed to access the internet unless it can declare ahead of time the contents of what it is going to make.

In other words, there are:

  • "fixed output" builds (we call them "derivations") where it downloads something from the internet. To have a fixed output derivation, it has to start out by saying what the sha256 of the result is going to be. If what it creates doesn't match the sha256, the build is failed and the result is deleted. In this case, before starting the inputs are unknown (the network!) so the output must be known.
  • standard builds where there is no network access, but the output is not limited. In this case all the inputs are known ahead of time, so the output doesn't have to be known.

This is part of how Nix accomplishes its reproducible build guarantees.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, Nix separates download phase from build phase.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Got it. I suspect this process could be made a fair bit prettier in 0.19.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed - I'm checking every week for an alpha :)

debounce_cfg =
Debounce.config
.debounce_state
-- getState : Model -> Debounce.State
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think putting the comments on the preceding line will clarify things, and make the "trailing" comment thing go away


view : Model -> Html Msg
view model =
if List.length model.options > 0 then
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

of options could get very long, if not <| List.isEmpty model.options then may be clearer/safer. length is O(n)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

options is about 5,000 items and will probably only go up (but slowly.)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd either write this using if List.isEmpty model.options then, or using a case..of

case model.options of
    [] ->
        do stuff

    _ ->
        do other stuff

(List.concat (List.map (optionToTd model.selected) (fetch_page model.page model.matchingOptions)))
]
, ul [ class "pager" ]
[ li []
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could potentially format these a bit different (i.e. li [] [ (changePageIf (not (model.page <= 1)) 1 "« First") ] could go on a single line, pretty sure elm-format would allow it, too :)

(if (List.length model.matchingOptions) == 0 then
"Showing no results"
else
(String.concat
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specific reason you seem to (generally) prefer List.concat and String.concat over the ++ concatenation operator? ++ feels little more idiomatic, to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignorance :)

update_options : Model -> NixOptions -> Model
update_options model unsorted_options =
let
options =
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General-ish remark: type signatures everywhere prevents a lot of mistakes down the line; even in let..in expressions :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Way!


selected =
QueryString.one QueryString.string "selected" qs
in
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like there's some shared code between updateFromUrl and init..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Thanks!

newModel =
update_options model options
in
( newModel, updateUrl newModel )
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps an alternative is to rewrite updateUrl so it's type-signature can be updateUrl : Model -> (Model, Cmd Msg). If you do that, then these branches can be written as model |> update_options options |> updateUrl. (you'd have to flip the params for updateOptions, but in general putting the "core" datastrcture a function works on as the last argument is good form, since it's pipeline-friendly)

)


nix_string : String -> String
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me a while to realize this, but what's with the snake_case functions? This feels very weird. Some sort of disambiguation between exposed and non-exposed functions? I'd say that doesn't really make sense - whether a function is exposed or not doesn't make a difference to how you'd use it within the defining module.

I feel like I'm missing something?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it wasn't clear to me how I should name my functions, and then effectively picked at random 😳 I'll clean these up :) thank you!


getOptions : Cmd Msg
getOptions =
Http.send FetchedOptions (Http.get "./options.json" decodeOptions)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case (and quite a few others), I personally like using <| to remove some parents:

Http.send FetchedOptions (Http.get "./options.json" decodeOptions)
Http.send FetchedOptions <| Http.get "./options.json" decodeOptions

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, perhaps, even more sensical

Http.get "./options.json" decodeOptions
    |> Http.send FetchedOptions

i.e. build the request, then send it

@zraexy
Copy link
Member

zraexy commented Sep 14, 2017

Although the debouncing does help some, a longer delay is needed in order to be sure that typing has finished. Currently, any hesitation whatsoever causes a new entry to be pushed into history; I think it should at least wait for 1/3 of a second. Unfortunately, the delay cannot be increased too much as it would feel sluggish. I'm honestly not sure that it's even possible to strike a good balance with the current setup.

I noticed something else: While the old search requires that all terms are found in order for the result to be included, this only requires that one is. Is that intentional?

@grahamc
Copy link
Member Author

grahamc commented Sep 14, 2017

I think I feel comfortable with extra history pushes, as it is far from every character (your original bug report) and does have some utility to it. Is this okay?

AND for OR: No, this wasn't intentional :) Fixing...

List.any (term_matches name) terms
|| List.any (term_matches option.description) terms
List.all (term_matches name) terms
|| List.all (term_matches option.description) terms
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, so now (all terms appear in name) OR (all terms appear in description). Is that the intended semantic?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like you want:

List.all (\term -> term_matches name term || term_matches option.description term) terms

This ensures every term is either in the name or description.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Got it! Thank you!

nixos/options.html: elm

elm: elm-src/index.elm
nix-build ./elm-src --out-link ./elm
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling nix-build here breaks calling the Makefile from a Nix expression (as we do e.g. in https://github.com/NixOS/nixops/blob/master/examples/nix-homepage.nix).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But ... what about all the other nix-build calls in the Makefile? 😕

@edolstra
Copy link
Member

Hm, I'm not super-enthusiastic about. It replace something that already worked with something that is significantly more complex (e.g. elm-src/default.nix), and the Elm code is actually longer than the Javascript code it replaces.

Also, I note that elmPackages.elm has a 1.6 GiB closure size. nixos.org doesn't have a lot of disk space in /nix so this is a pretty heavy dependency.

@domenkozar
Copy link
Member

and the Elm code is actually longer than the Javascript code it replaces.

Yeah Elm is a bit more verbose, but it's a pure langauge so it doesn't fail at runtime (except when you compare functions).

Also, I note that elmPackages.elm has a 1.6 GiB closure size. nixos.org doesn't have a lot of disk space in /nix so this is a pretty heavy dependency.

This is because Elm has multiple executables, we could actually fix that now that haskell multiple-outputs were merged. Elm 0.19 (due to be released soon) will have one executable and the closure should be ~50MB.

@grahamc
Copy link
Member Author

grahamc commented Sep 18, 2017

We'll revisit this when Elm 0.19 comes out.

Copy link

@jb55 jb55 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, just a random drive-by Elm code review 👍


mkGsonList : List Gson -> Gson
mkGsonList data =
GList (List.map (\x -> x) data)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

List.map (\x -> x) data is equivalent to data

[ "\""
, key
, "\" = "
, (nixFromGson value)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: redundant parens

pos =
List.Extra.findIndex matcher options
in
case pos of
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole case could be replaced by:

Maybe.map (\p -> ceiling (toFloat p / 15.0)) pos

jerith666 added a commit to jerith666/elbum that referenced this pull request Nov 28, 2017
jerith666 added a commit to jerith666/elbum that referenced this pull request Dec 1, 2017
@domenkozar
Copy link
Member

0.19 is out :-)

@davidak
Copy link
Member

davidak commented Aug 14, 2019

What's the status?

My opinion is that the nixos website code should be consistent to be maintainable, so use as few dependencies as possible. Don't use JavaScript and Elm, use only one of them. Since JS is more popular, we might easier find contributors, but i understand that Elm has other advantages. Just don't make a mess of the project and keep it maintainable.

Maybe adding a new language/technology could go through the RFC process?

@grahamc
Copy link
Member Author

grahamc commented Aug 14, 2019

I thought I had closed this already.

@grahamc grahamc closed this Aug 14, 2019
@grahamc
Copy link
Member Author

grahamc commented Aug 14, 2019

Also, I don't think the website is under the RFC process right now.

fricklerhandwerk added a commit to tuxiqae/nixos-homepage that referenced this pull request Oct 11, 2022
this is easier to read, especially within a number
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.

None yet

8 participants