MithrilJS / mithril.d.ts

Types for mithril.js
MIT License
79 stars 11 forks source link

Changes to file structure to also support using Mithril globally #36

Open felix-roehrich opened 5 years ago

felix-roehrich commented 5 years ago

I propose changing the file structure, so this type declaration, also supports global usage.

Benefits:

Changes:

For the new usage you can take a look at this minimal example

P.S. I am using GoLand, so some feedback on other IDEs would be appreciated, though I expect it to work anywhere.

Edit: The namespace can be renamed Mithril -> m, so you can still write m.Vnode instead of Mithril.Vnode.

spacejack commented 5 years ago

We would have liked to make a unified global/module definition file however the way Mithril's stream package works in a script/global context makes this problematic. When the stream script is added to a page, it attaches itself to the global m object. So you have m.stream. If the global mithril script is not present (i.e., the user did not include mithri.js), then stream creates its own (empty) global m object to attach stream to. This is why we have separate global/module typedefs.

felix-roehrich commented 5 years ago

I understand that if you use require you have to declare both mithril and mithril/stream, but in global context you expect it as part of m. So, if you simply do not want stream as part of m if you use require then this example should work.

spacejack commented 5 years ago

Stream is not automatically included with mithril.js, you have to add the stream.js script to your html. So the core mithril types and stream types are separate.

The module types are easy, it's the global ones (which conditionally tack-on stream to m) that take extra work to accommodate.

felix-roehrich commented 5 years ago

I know that Stream is not automatically included. I do not know if you took a look at my example but I cannot understand your problem. In my example the typedefs are still separate (though there is a reference to the stream types), but in mithril-global Stream is also part of the namespace by default. So maybe you just draw the line (on what means separate) different from me.

Edit: I think it is possible to make unified typedef. It does not have to be like in my example (I also got some other ideas) but as long as I do not understand what your problem exactly is, I cannot offer other suggestions.

spacejack commented 5 years ago

There are 3 separate cases that need to work (in a global context):

  1. Mithril only (no Stream types included)
  2. Stream only (no Mithril types, only Stream - attached to a m global)
  3. Mithril and Stream both on the m object
felix-roehrich commented 5 years ago

Your description and the current API seem to imply that different structure (which would affect Mithril package itself) is more appropriate (because Core and Stream are/seem of equal value):

If somebody only wants the stream/core component they can require it as mithril/stream or mithril/core; for both components mithril. (I will address the global version below.)

With this approach, you would clearly separate Core and Stream components and could get the require/global version in one unified typedef. As mentioned above this seems to be the most reasonable move, though you might disagree.

Finally, and this is probably annoying for you (as it is for me), I simply do not understand your reasoning: I understand that there are three separate cases that need to work independently from one another, but the current mithril-global typedef does not distinguish between these cases. If you want the stream/core typedefs for require and global completely separate, then you might want to serve the core component as mithril and the stream component as mithril-stream (for package and typedef). With this approach you can use augmentation, such that, if only mithril is installed (via npm), the m global contains only the core, if only mithril-stream is installed, m only contains the stream components, and if both are installed, it contains both components. (Needless to say people using the CommonJS approach require the respective package.)