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

Adding index file to include all of trine without needing to include individual files #46

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ghost
Copy link

@ghost ghost commented Jul 20, 2015

No description provided.

@jussi-kalliokoski
Copy link
Owner

Thank you for taking the time to contribute!

This should be as a generated file instead of being done at runtime, in order to support browser environments as well. Also the modules need to be grouped under the categories they're in, because there might be overlap in the names, e.g.

import Trine from "trine";

({ foo: bar })::(Trine.object.entries)()
(...)::(Trine.someOtherType.entries)()

EDIT: Which makes me question the ergonomics of this feature...

@nikhedonia
Copy link
Contributor

maybe just for each module?

import { values , entries } from "trine/object"
//...

@jussi-kalliokoski
Copy link
Owner

maybe just for each module?

that seems like a good option! 👍

@jussi-kalliokoski
Copy link
Owner

But different types of data often have similar methods that require different implementations - this requires either name collisions or overloading (which both contribute to mental overhead). I personally prefer name collisions because it's more explicit and leaves the developer in control, instead of imposing branches that are not necessarily needed. Consider #49 for example:

Lodash/Underscore have a pick method that returns a new object containing only the specified keys. Now, let's say we want to also be able to pick Map instances.

  • Option 1: Add a pick method to each, so you can do
import { pick as pickFromObject } from "trine/object/pick";
import { pick as pickFromMap } from "trine/map/pick";
import { pick as pickFromArray } from "trine/array/pick";

({ x: 1, y: 2 })::pickFromObject(["y"]) // { y: 2 }
new Map([["x", 1], ["y", 2]])::pickFromMap(["y"]) // Map([["y", 2]])
[1,2]::pickFromArray([1]) // [2]
  • Option 2: Add an iterable method pick that works on all collections, but requires additional transforms:
import { pick } from "trine/iterable/pick";
import { to } from "trine/iterable/to";
import { entries } from "trine/object/entries";

({ x: 1, y: 2 })::entries()::pick(["y"])::to(Object) // { y: 2 }
new Map([["x", 1], ["y", 2]])::pick(["y"])::to(Map) // Map([["y", 2]])
[1,2].entries()::pick([1])::to(Array)[0][1] // [2]
  • Option 3: Add overloads to the method pick, with runtime and size cost (as well as uncertainty of what exactly the method accepts and returns):
import { pick } from "trine/any/pick";

({ x: 1, y: 2 })::pick(["y"]) // { y: 2 } or [["y", 2]] ?
new Map([["x", 1], ["y", 2]])::pick(["y"]) // Map([["y", 2]]) or [["y", 2]]?
[1,2]::pick([1]) // [2] or [1] or [[1, 2]] or [[0, 2]]?

@jussi-kalliokoski jussi-kalliokoski added this to the 0.2.0 milestone Jul 27, 2015
@jussi-kalliokoski
Copy link
Owner

You're adding ambiguity for the sake of what, exactly?

There's no ambiguity currently: import { pick } from "trine/object/pick" or import { pick } from "trine/object" is explicit on what data type it operates on. Naming methods that do the same thing but on different data types with the same name is common and useful for not having to repeat yourself as well as consistency. You wouldn't want Number.prototype.toString to be Number.prototype.numberToString either, no? In a similar fashion, object/pickFromObject is useless repetition.

For example in the case of pick, it's unlikely that you'll need the method for multiple data types within one module, so it's just optimizing for the worst case scenario to avoid using the same name for the semantically same thing. If we optimize for the common scenario instead with option 1 and index modules as @nikhedonia proposed, it still leaves the developers faced with the uncommon case in control of the situation by allowing them to rename the bindings in their modules (and also encourages usage of direct references to the modules instead of importing things you don't need).

@jussi-kalliokoski
Copy link
Owner

(please don't prefix numbers with # to avoid referencing issues :)

it's unlikely that you'll need the method for multiple data types within one module

That's a very bold statement and I definitely do not agree with it.

I guess that's just my bias for small focused modules speaking, then. :) Wikipedia's definition for modularity:

Modularity is the degree to which a system's components may be separated and recombined.

If a single JS module exports more than one functionality, it means that even if you want only one of the exports, you get all of them, which breaks the "may be separated" part. Same goes for methods with overloads (by definition, overloading means it does more than one thing, e.g. "returns a new object with only the specified fields of the original object" vs. "if object, returns ... if map, returns ... etc.") - if you need the method for only one type, having a method with overloads forces you to take them all and pay the costs that come with it. This is why I prefer modules that export only one function (other things, such as constants that are required for working with that function are OK IMO). In such modules, I can't really think of a case where you'd need more than one version of pick for example.

This conversation is actually making me think that having index modules in Trine at all is a bad idea. :D Making bad choices (importing all of Trine) easier than good ones (importing only the things you need from Trine) usually tends to lead in a lot of people going with the bad choices. What I'd like to see is better module discoverability features in editors, e.g. you type import { pick } and it suggests from "trine/object/pick", from "trine/map/pick", etc. That way good choices will be encouraged by good ergonomics, instead of ergonomics dictating the quality of choices.

@jussi-kalliokoski
Copy link
Owner

Libraries shouldn't attempt to enforce coding practices.

I'd use the word encourage - and why not? It's good to encourage good practices, no? And it's just what you'd expect from a library. You don't go into a real library and take every book with you because you're too lazy to figure out which one you want to read either. It's not because you wouldn't like to have all those books available to you in your living room, but because it would cause you inconvenience to do so. However if it was actually easier than just taking the books you need, people would do it, regardless of how it makes all those books unavailable to everyone else.

I'd much rather see

I'd rather not see either because if a single module requires that many things, the module is too big and does too many things / skips abstraction levels which is bad for modularity.

You need to know the inner workings of the nested folders as opposed to just the end-points.

You shouldn't need to know either - discoverability can and should be fixed by proper code editors.

And what do you mean by "importing all of trine"? import * from "trine"? If so, so what? It causes extra typing obj::trine.method() and it doesn't hurt anything.

No, I mean that import { foo } from "trine"; requires loading in the code of every function in Trine, and it does hurt. It hurts the users having to wait for all that unused code to download because the developer was lazy. Performance matters and I'd rather not create a conflict of interest between developer experience (DX) and user experience (UX); they should be aligned.

@barneycarroll
Copy link

Very solid reasoning @jussi-kalliokoski

Tangent on an earlier point: it's perfectly possible to

import arrayPick from 'trine/array/pick'
import objectPick from 'trine/object/pick'

And of course it's trivial for any author using modular Javascript to create their own wrappers, eg

export default function pick(){
  return this::(
    this instanceof Array
      ? arrayPick
      : objectPick
  )( ...arguments )
}

@jussi-kalliokoski
Copy link
Owner

Not when you're forcing the user to include files one-at-a-time

No, I'm not forcing anything. Anyone has the freedom of choice to not use this library, or to make a wrapper around it that exposes everything in a single module. And even if one ends up in charge of a codebase that uses Trine and doesn't like it, the modularity allows them to refactor and replace it with something else without an intermediary state of two gargantuan libraries in the codebase.

Do you see:
...
? Or from jQuery, or underscore, react, angular, rxjs, etc....

Yes, lodash recently added support for this and it is now the recommended way of using lodash for minimal footprint. As for

  • jQuery: its API is designed in the time before any kind of module system other than global scope, and it's so unmodular that 1) Zepto and other lighter / more modular versions were created 2) angular <v2.0.0 ships with its own jQuery lite 3) it's recommended to make a custom build.
  • underscore: the lack of custom builds was one of the reason lodash was born, and underscore is also considering making the same move as lodash.
  • React: in v0.14 splits rendering backends and addons into a separate module, and more separation will likely follow.
  • Angular: the current situation is a mess with the custom module system and they're making a complete pivot with v2.x - haven't checked yet how they deal with modules in 2.0.0.
  • rxjs: their monolithic design choices have also resulted in people reinventing the wheel with their own, lighter versions of the library.

your preference should not attempt to dictate how people use a library.

My differing preference is why I created the library in the first place. Lodash/underscore, prototype.js and Ramda also impose their own preferences on the users and if I agreed with those this library would be a pointless (and possibly even harmful) clone of one of them. That's not to say I want this library to be based on my preferences only, discussion about different preferences changes and leads to better understanding of one's own preferences as well, like this discussion has made me consider this feature to be a bad idea (while I originally thought of including it in the initial release).

There's no overhead there, so what's wrong with a runtime include?

Ever waited for npm install while it was downloading what seems to be the whole internet? This is why #47 was filed and why lodash also now ships each method in a separate module. Unused code being loaded is also why things such as load-grunt-tasks exist.

I should be able to edit my code in vi, or sublime, or atom, or notepad if I want.

Sure you should, and those editors should provide the tooling to help make it easy for you.

I shouldn't have to depend on an editor to be able to use something

You don't. Having a better editor just makes it easier to code. I code roughly 50% in Vim and 50% in Atom (shifting more and more towards Atom as it's become more suitable to my preferences and supports more modern features), and I think both of them should support module discoverability, otherwise they're going to become more and more annoying to use compared to editors that help their users more. Currently the best method of discoverability most libraries have are their docs, and as such Trine's docs tell you how to import each and every endpoint.

And even if you advocate that, it's not ready yet!

Neither is dead code elimination for unused exports and not having that affects more people (unless the only users of the application using the library are the developers). The tooling is there (flow/tern), all editors need to do is integrate them with good UX.

So tooling is responsible for code discovery, but not dead code removal? ;)
...
Or you include modules and tools include necessary code. import { foo } from "trine" in my build environment will only include "foo".

Is this hypothetical? If not, I'd love to know what build environment you're using. This kind of dead code elimination is an extremely hard problem to solve, especially with requirements of async loading and code splitting (method A of module B is not used in module C, but how about in module D that is loaded later?). That's not to say I wouldn't love to see such a feature. Code discoverability on the other hand is a simple and solved problem, the only problem is that editors don't take advantage of it.

@barneycarroll
Copy link

Maybe worthwhile authoring a trine-bundle package which offers every Trine package as a method, with a type detection on this for forking homonyms?

@jussi-kalliokoski
Copy link
Owner

@barneycarroll

Maybe worthwhile authoring a trine-bundle package

Or maybe trine-bloated. :P Jokes aside, if we decide to make it an official feature, might as well put it in the main package as the main module.

@zyklus

it is/was slow

The startup is still relatively slow, usually because some plugin requires a whole bunch of code it doesn't use. Performance matters.

a resource hog and kills battery life. For me Atom is worse than the baseline in ways that matter to me.

Battery life is an important thing, and while detached – especially on mobile – network is usually one of the biggest battery life hogs. By sending only the necessary code over the network (be it a website or npm install) you're conserving battery life, for you and your users (not to mention money for people without an unlimited data plan).

Basically, your reasons for not using atom are the reasons why I think this feature is a bad idea.

forcing me to know the internal structure of a library in order to use it is worse than the baseline

If you don't know what the library contains, how do you figure out what to import anyway or end up using the library in the first place? By looking at the docs? If so, the docs immediately expose you to the internal structure.

Now let's consider some use cases of working with an existing codebase.

  1. Take a new method you know beforehand into use in an existing file that already imports something from the library.
  2. Figure out if the library has a method fitting your needs before implementing the method yourself.
  3. Same as 2 except you're too lazy to look up docs or are offline.

Single module library, use case 1

  • Find the import statement of the library.
  • Amend it with the new method.

Single module library, use case 2

  • Open docs and find the method and its description.
  • Take it into use as described in the docs.

Single module library, use case 3

  • Open the library main module in editor.
  • Look at the defined imports.
  • Based on name, maybe this one...
  • Find for the method definition. Eh, not defined in this file, required from somewhere else.
  • Open that file in editor.
  • Find the method definition. Oh, ok, This is actually an overload wrapper for multiple methods. The type we're looking for is object, and that implementation is required from somewhere else.
  • Open that file in editor.
  • Find the method definition. Look at method signature and docblock to figure out if it fits.
  • import { foo } from "library";

Split module library, use case 1

Same as for single module library.

Split module library, use case 2

Same as for single module library.

Split module library, use case 3

  • Let's see what this thing contains... ls -R node_modules/trine (or open in a file browser if you prefer GUIs).
  • Ok, each method seems to be in its own file under the type signature it has, we're dealing with objects so let's look under object...
  • Based on name, maybe this one.
  • Open file in editor.
  • Find the method definition (there's only one). Look at method signature and docblock to figure out if it fits.
  • import { foo } from "library/object/foo";

Even without editor support, what's the advantage of the single module?

I don't want to be annoying here or downplay the problem you're having, however before making it easy to do things that demonstrably have bad consequences, I want to get to the bottom of the problem that the feature is supposed to solve. What exactly is the inconvenience? Once we have that information, we may notice that there may be better solutions to the problem, such as improving the documentation.

@jussi-kalliokoski
Copy link
Owner

I'd also note that the example you had in your comment does not reflect any realistic case because method names are more often than not more than a single character long, so the same with actual names:

import { dropTail, dropHead, intersection, sortAlphabetically, uniq, toFunction, partial, difference, enumerate, zip } from "trine";

or, more realistically spread along multiple lines:

import {
    dropTail,
    dropHead,
    intersection,
    sortAlphabetically,
    uniq,
    toFunction,
    partial,
    difference,
    enumerate,
    zip,
} from "trine";

versus

import { dropTail } from "trine/iterable/droptail";
import { dropHead } from "trine/iterable/dropHead";
import { intersection } from "trine/iterable/intersection";
import { sortAlphabetically } from "trine/iterable/sortAlphabetically";
import { uniq } from "trine/iterable/uniq";
import { difference } from "trine/iterable/difference";
import { enumerate } from "trine/iterable/enumerate";
import { zip } from "trine/iterable/zip";
import { toFunction } from "trine/value/toFunction";
import { partial } from "trine/function/partial";

Now the comparison doesn't seem that bad, and as an added bonus you see the input types of the methods.

@tomek-he-him
Copy link

Wow, that was a lot to read throgh.

@zyklus, I have to admit I don’t understand your frustration – sometimes even brushing against arrogance in my view. I can recommend an interesting conversation I held with the creator of module-best-pracitces. It led me to change my point of view and prefer modularity even more radical than import {zip} from 'trine/iterable/zip' – tipping it towards import zip from 'some-tiny-zip-library'.

module-best-pracitces is itself a great read I highly recommend.

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.

4 participants