ianmackenzie / elm-3d-scene

A high-level 3D rendering engine for Elm, with support for lighting, shadows, and realistic materials.
https://package.elm-lang.org/packages/ianmackenzie/elm-3d-scene/latest/
Mozilla Public License 2.0
207 stars 27 forks source link

Consider Namespacing library dependencies #40

Closed ChristophP closed 4 years ago

ChristophP commented 4 years ago

Problem elm-3d-scene makes use of bunch of other libraries. A couple of them are not namespaced for people (like me) who haven't used those before it is difficult to see where the imports come from.

import Angle -- ianmackenzie/elm-units
import Camera3d  -- ianmackenzie/camera-3d
import Color -- tesk9/palette
import Color.Transparent -- tesk9/palette
import Direction3d -- tesk9/palette
import Length -- ianmackenzie/elm-units
import Pixels exposing (Pixels, pixels) -- ianmackenzie/elm-units
import Point3d -- ianmackenzie/elm-geometry
import Scene3d exposing (noDirectLighting, noEnvironmentalLighting) -- ianmackenzie/elm-3d-scene
import Scene3d.Chromaticity as Chromaticity -- ianmackenzie/elm-3d-scene
import Scene3d.Drawable as Drawable -- ianmackenzie/elm-3d-scene
import Scene3d.Exposure as Exposure -- ianmackenzie/elm-3d-scene
import Scene3d.Mesh as Mesh -- ianmackenzie/elm-3d-scene
import Triangle3d -- ianmackenzie/elm-geometry
import Viewpoint3d -- ianmackenzie/camera-3d

Suggested Solution Namespace the modules in the other libraries like Unit.Length instead of Lenght for elm-units. Other affected libraries are elm-geometry, elm-3d-camera and possibly more.

Other Implications Namespacing would also help dodge the fact that the compiler currently cannot diambiguate modules with the same name. See here

Other If there is a decision to implement what this issue suggests, it may be better to track on the respective repos of the libraries. But until them I felt it would give a better insight over the issue here.

ianmackenzie commented 4 years ago

elm-geometry (well, really its precursor opensolid/geometry) actually used to have prefixed module names, but I moved away from that a while ago in favor of plain Point3d, Triangle3d etc. A bunch of the relevant discussion is in https://github.com/ianmackenzie/elm-geometry/issues/22, but the short version is that non-prefixed module names seemed to be the community standard and that there are probably better ways to avoid the naming collision issue than package authors adding prefixes everywhere. (Personally, I think it would be nice if modules could optionally be imported with a syntax like import Point3d from ianmackenzie/elm-geometry as Pt3d, but there are probably lots of other potential solutions). I'm also not aware of any current module naming conflicts with any of my packages, so I don't think an urgent fix is needed.

I agree that it can be confusing figuring out which packages different imports come from (you're not the first and probably not the last person to bring this up), but I'm not particularly eager to reopen the prefixing/namespacing discussion right now. What probably would make sense, though, would be to annotate the examples much like you've done above, so that people reading the examples can immediately see what's happening. Thoughts?

ChristophP commented 4 years ago

Hi Ian, I see it actually used to be that way that the modules were namespaced. Given the history I understand you don't wanna reexamine everything again.

Annotating examples: I think that would solve the confusion for beginners. The only problem with it is that once you run elm-format over it, it will pull all the comments up over the block of imports. I described it in this issue here.

Naming collisions: Totally agree that having something like import X from author/user would be great to disambiguate, but the last issues I've seen haven't really moved since 2017. There may not be current clashes YOUR packages, but I could still see some problems ahead, because the name clashing also affects indirect dependencies. This means that for example no one using elm-3d-scene will be able to use any other Color module than tesk9/pallete. Would be a shame if people had headaches because their dependencies don't line up with elm-3d-scene's. I although I realize now, that even namespacing everything will not solve this problem. :-/

ianmackenzie commented 4 years ago

Good point about the elm-format issue - that's a bit annoying, but nothing that can't be worked around if necessary.

Are you sure that name clashing also affects indirect dependencies? I don't think that's true...I mean, you certainly get compile errors if you try to import a module in a package you only indirectly depend on (it's happened to me several times that I get a compile error for import Json.Decode because I haven't explicitly installed elm/json, even though it's in my indirect dependencies). To me that implies that something like import Color would work as long as I directly depended on only one of the color packages, even if I had the other as an indirect dependency somehow, since it certainly seems that the compiler hides modules from indirect dependencies during the import resolution process.

I also think namespacing at this point is a short-term fix that might avoid some issues in the near term while at a potential cost to the Elm language/ecosystem in the long term. You're right that there hasn't been much recent movement on things like https://github.com/elm/compiler/issues/1625, but until Elm gets close to 1.0 I think things like module import syntax should still be considered to be in flux. And in order to figure out the best solution, Evan et. al. need case studies of where there are issues with the current design...such as instances where naming conflicts actually come up! So any naming conflicts avoided right now by adding a namespace would mean fewer case studies to consider, making it harder to figure out the right long-term solution. On the flip side, if not adding a namespace now means naming conflicts do come up, then there's a bit of pain to work around in the near term, but it provides a good data point for figuring out what the long term module import solution should be.

ChristophP commented 4 years ago

Are you sure that name clashing also affects indirect dependencies?

Now that you put it that way I am not 100% sure. You may be right about that. If it doesn't affect dependencies that is a very good thing. I may have confused that case with versions because the compiler can not resolve multiple version of the same library as far as I know.

I agree with the rest of your statement, that namespacing now will prevent gathering data points for a proper fix in the language.

On the flip side, if not adding a namespace now means naming conflicts do come up, then there's a bit of pain to work around in the near term

Not sure if what that bit of pain would be, because I think once you have those issues there's not really anything you can do to my knowledge besides namespace then to avoid it.

Having heard your reasoning, I think it's fine to leave module names as they are for now. Since we done quite some "Considering" I think we can close the issue. :-)

ianmackenzie commented 4 years ago

You're definitely right about versions - indirect dependencies can certainly cause version constraint conflicts. (And of course if you use ianmackenzie/elm-3d-scene then you'll probably have to directly depend on tesk9/palette yourself anyways, which means you will get module import conflicts if you also depend directly on avh4/elm-color.)

I think it would be possible to work around things in some cases by vendoring certain dependencies into your own repository and placing them under a prefix there; for example you could depend on elm-3d-scene and then copy the avh4/elm-color source files into your own repository under a prefix or something. You could also do something like publish a forked version of elm-3d-scene that used avh4/elm-color or used a vendored/prefixed version of tesk9/palette or something. Perhaps describing that as "a bit" of pain is pushing it, but I think there are workarounds in at least some cases.

Anyways, yes, I think we're all pretty clear on what the issues/tradeoffs are so I think I'll close this for now. If at some point it becomes clear that import resolution is not going to change, then I'm happy to reopen the discussion - although ideally as part of a broader conversation within the Elm community, so that there can be some consistency between packages.