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
Conversation
Of course keep in mind I built this to learn elm, not specifically with the
intent of it merging and being used for NixOS. If you like it, I'll address
feedback and make it mergable. If not, it won't hurt my feelings and you
can close the PR :)
…On Mon, Sep 4, 2017 at 11:29 PM Graham Christensen ***@***.***> wrote:
Just a last minute PR before vacation. Incorporates Elm in to the build
phase, including pinned elm dependencies. PR version of
http://gsc.io/elm-thing/?selected=boot.initrd.luks.devices
I'll be back in a week if you have questions :)
------------------------------
You can view, comment on, or merge this pull request online at:
#160
Commit Summary
- Build the options page in Elm
File Changes
- *M* Makefile
<https://github.com/NixOS/nixos-homepage/pull/160/files#diff-0> (8)
- *A* elm-src/.gitignore
<https://github.com/NixOS/nixos-homepage/pull/160/files#diff-1> (2)
- *A* elm-src/NixJSON.elm
<https://github.com/NixOS/nixos-homepage/pull/160/files#diff-2> (150)
- *A* elm-src/NixOptions.elm
<https://github.com/NixOS/nixos-homepage/pull/160/files#diff-3> (50)
- *A* elm-src/Paginate.elm
<https://github.com/NixOS/nixos-homepage/pull/160/files#diff-4> (26)
- *A* elm-src/default.nix
<https://github.com/NixOS/nixos-homepage/pull/160/files#diff-5> (77)
- *A* elm-src/elm-package.json
<https://github.com/NixOS/nixos-homepage/pull/160/files#diff-6> (21)
- *A* elm-src/elm-stuff/exact-dependencies.json
<https://github.com/NixOS/nixos-homepage/pull/160/files#diff-7> (14)
- *A* elm-src/index.elm
<https://github.com/NixOS/nixos-homepage/pull/160/files#diff-8> (414)
- *M* nixos/options.tt
<https://github.com/NixOS/nixos-homepage/pull/160/files#diff-9> (340)
Patch Links:
- https://github.com/NixOS/nixos-homepage/pull/160.patch
- https://github.com/NixOS/nixos-homepage/pull/160.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#160>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAErrMnLBTZBk7OPZy_fgS0JGs8RTt1Lks5sfMAUgaJpZM4PMZqC>
.
|
Looks good. The only issue I see is that it introduces a couple of regressions:
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 |
Good catch!
Not sure how to fix this with Elm, but I'll research when I get back.
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. |
@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. |
I've found an issue when your query has no results. Do you see other issues?
…On Tue, Sep 12, 2017 at 9:36 PM Graham Christensen ***@***.***> wrote:
@zraexy <https://github.com/zraexy> can you test again?
gsc.io/nixos/nixos/options.html code needs cleaning up, but should work
ok now w.r.t. back buttons and history pollution.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#160 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAErrBvhMep80egsnN4F7bzCERkqClVSks5shzGlgaJpZM4PMZqC>
.
|
elm-src/NixJSON.elm
Outdated
nix_list list | ||
|
||
GDict dict -> | ||
if ((Dict.member "_type" dict) && ((Dict.get "_type" dict) == Just (GString "literalExample")) && (Dict.member "text" dict)) then |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Thank you!
elm-src/NixJSON.elm
Outdated
entries = | ||
Dict.toList dict | ||
in | ||
if List.length entries == 0 then |
There was a problem hiding this comment.
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
elm-src/NixJSON.elm
Outdated
[ [ "{\n" ] | ||
, indent | ||
(List.concat | ||
(List.map nix_enc_dict entries) |
There was a problem hiding this comment.
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
elm-src/NixJSON.elm
Outdated
|
||
nix_list : List Gson -> String | ||
nix_list list = | ||
if List.length list == 0 then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List.isEmpty
There was a problem hiding this 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 |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
elm-src/index.elm
Outdated
debounce_cfg = | ||
Debounce.config | ||
.debounce_state | ||
-- getState : Model -> Debounce.State |
There was a problem hiding this comment.
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
elm-src/index.elm
Outdated
|
||
view : Model -> Html Msg | ||
view model = | ||
if List.length model.options > 0 then |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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
elm-src/index.elm
Outdated
(List.concat (List.map (optionToTd model.selected) (fetch_page model.page model.matchingOptions))) | ||
] | ||
, ul [ class "pager" ] | ||
[ li [] |
There was a problem hiding this comment.
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 :)
elm-src/index.elm
Outdated
(if (List.length model.matchingOptions) == 0 then | ||
"Showing no results" | ||
else | ||
(String.concat |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignorance :)
elm-src/index.elm
Outdated
update_options : Model -> NixOptions -> Model | ||
update_options model unsorted_options = | ||
let | ||
options = |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Way!
elm-src/index.elm
Outdated
|
||
selected = | ||
QueryString.one QueryString.string "selected" qs | ||
in |
There was a problem hiding this comment.
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
..
There was a problem hiding this comment.
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 ) |
There was a problem hiding this comment.
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)
elm-src/NixJSON.elm
Outdated
) | ||
|
||
|
||
nix_string : String -> String |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
elm-src/index.elm
Outdated
|
||
getOptions : Cmd Msg | ||
getOptions = | ||
Http.send FetchedOptions (Http.get "./options.json" decodeOptions) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
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? |
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... |
elm-src/NixOptions.elm
Outdated
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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? 😕
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 |
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).
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. |
We'll revisit this when Elm 0.19 comes out. |
There was a problem hiding this 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) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
hopefully this will work better once 0.19 is out see https://github.com/gdotdesign/elm-github-install and/or NixOS/nixos-homepage#160
hopefully this will work better once 0.19 is out see https://github.com/gdotdesign/elm-github-install and/or NixOS/nixos-homepage#160
0.19 is out :-) |
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? |
I thought I had closed this already. |
Also, I don't think the website is under the RFC process right now. |
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 :)