elm-mdl icon indicating copy to clipboard operation
elm-mdl copied to clipboard

Change component indexes inside model to String or comparable

Open doodledood opened this issue 8 years ago • 10 comments

Currently some components need an integer index to render in order to find their model. This should be changed to a String type instead mainly because the of cognitive burden associated with keeping track of unique numbers instead of unique meaningful names.

doodledood avatar Aug 19 '16 00:08 doodledood

@vipentti, nice PR debois/elm-parts#8!

If we integrate this in elm-mdl, users will have to decide on an index type when they define their model and messages, right?

type alias Model = 
  { ...
  , mdl : Material.Model String
  }

 type Msg =
   ...
   | Mdl Material.Msg Msg String

Now that I see this, I'm a little concerned that users will get really complicated type errors if they don't use the same type in both places.

Any comments?

debois avatar Aug 22 '16 10:08 debois

Well, since this is a UI library, my opinion is that we should keep it simple and limit ids to strings to avoid this problem. Using a generic type here doesn't really offer much and opens a door for confusion.

Another option would be to keep the generic type but recommend using a custom union type that contains the unique ids (like elm-css does). Adds a bit boilerplate but now you have the compiler on your side, telling you when you created a non unique id. (probably will need to convert the type to string to use as id anyways)

doodledood avatar Aug 22 '16 10:08 doodledood

Very interesting, especially the bit about unique IDs. Can you provide a link to the elm-css implementation? Or an example use?

debois avatar Aug 22 '16 10:08 debois

See implementation of "(#)"

What they do is use a generic type and then convert it to a string inside their module

doodledood avatar Aug 22 '16 10:08 doodledood

@debois We could also define the type in Material.elm

-- Material.elm 

type alias Indexed m =
  Parts.Indexed String m

type alias Index =
  Parts.Index String

-- Then change all instances of Parts.Index(ed) to Material.Index(ed)

vipentti avatar Aug 22 '16 11:08 vipentti

I think it'd be nice to have this in v8.

I'm torn between on the one hand wanting a single API that never gives weird type errors and on the other wanting to support both existing users List Int usage and new (or converting) users List String usage. Not sure how to square this.

debois avatar Aug 24 '16 15:08 debois

My programmerize would prefer the method where I can specify the type. However it might be better to just pick String as the type for List String everywhere, it is both uniform and anything can be cast to string anyway. Plus it is easy to concat the strings in the list. The method above of accepting 'anything' and just toStringing it anyway would satisfy all cases though, but it seems code-smelly to me because Elm does not have native ability to do things like that due to its highly limited type system (looking forward to that being fixed, it is on their roadmap!) and it only works because toString is very unique in the language...

OvermindDL1 avatar Aug 24 '16 15:08 OvermindDL1

I say we "suffer" with integers, or, if we must, move to Strings with no automatic conversions. Strings are very flexible and easy to generate and understand.

z5h avatar Aug 24 '16 16:08 z5h

@z5h made this comment on #elm-mdl in the elm-slack, which I thought I'd repeat here (edited slightly):

Perhaps, but either could be messier. If you are rendering a list of elements, it's more natural to use an integer index—if you really want your context to be a string, just use a hash: [hash "mylist", 1] ... [hash "mylist, n] Not super pleasant, but ok. With String indices, you have to do: ["mylist", toString 1] ... ["mylist", toString 2]

debois avatar Aug 24 '16 21:08 debois

What I do currently, for comparison, is pass a 'pidx' (previous index) and just add on another index, and any time I have a view that goes deeper then I just add another on to it as I pass it in. So either works for me, integers as they are work fine.

OvermindDL1 avatar Aug 24 '16 21:08 OvermindDL1