-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Chopping Underscore into lots of tiny modules #2849
Conversation
Fortunately, it appears we don't need the `legacy` option anymore.
`npm run doc` failed with an inexplicable error about `docco` not being in the PATH. This fixed it.
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.
Okay, you have my blessing! 👑
My one request would be that we keep a single link in the website sidebar to the old, single-file full source version of 1.10.2, as "Underscore Classic". I’ve heard from a lot of folks over the years who have cut their teeth on JavaScript by reading and trying to understand that file, and I’d like to not have that experience lost in the sands of time...
Let’s do this!
(If you need my help with anything specific, in terms of website stuff or cutting releases, just let me know.)
Woooo! Thanks, this means a lot to me.
In case you missed it, the proposal as it currently stands has a link in the website sidebar to (a docco rendered version of) this monolithic build. The sidebar refers to this as the "single read" annotated source. It is not sorted by category (collection, array etcetera), but it is strictly ordered by dependency and it does have all the comments. Don't you think this can fullfill the same need?
Thanks! I will get back to you about this. |
Not entirely — I think it’s great, but that you lose quite a bit in what’s possible to do with explanatory prose, when you lose control of the ordering of the paragraphs. People also like to just view the (They used to be clearer, back at the beginning: https://cdn.rawgit.com/jashkenas/underscore/0.4.0/underscore.js) |
Not to counter your point (I agree this is a drawback), but I should mention that we can still influence the order by reordering the imports. It's not full control, but still some.
This is possible with either monolithic bundle, no problem.
I'm not worried. I value your opinion a lot and I don't mind delaying the merge if I can make the changes (even) more satisfying. Although I should mention that I'd prefer fixing the comments in the new monolithic bundles over including a link to an outdated version of Underscore.
I see what you mean. I could make another pass over the source in an attempt to approach that quality more, but maybe you want to do it yourself? As an aside, version 0.4.0 might be nice as a superlative of "classic", i.e., "archeological". |
You’re right — it would be much better if we improved the monolithic current comments, instead of linking to an old and dusty shelf copy. The best way to do this would be to make a tool that allows us to order the monolithic builds in any arbitrary ordering that we choose ... but the second-best way to do that would be to re-write the comments in such a way that they flow naturally based on the actual ordering present in the monolithic build. That’s hard, given that they need to make sense in isolation, without reference to anything surrounding them ... and also make sense when read from top to bottom (this is the concern that you mention in your issue description above), but I guess that’s the challenge that presents itself in front of us. |
Credits to @cambecc for pointing these out.
Quick update: I'm currently in the process of analyzing the dependency graph of the modules, in order to choose the most comfortable import order. |
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.
Besides a couple of minor fixes, there are two main things I changed since the previous review:
- After creating a dependency graph and taking a long hard stare at it, I changed the order of the internal imports, in order to give the monolithic build an order that is more natural to read. It turns out that just reordering the
index.js
is already nearly sufficient to accomplish this; I needed only a few minor tweaks in other modules. This is good news for future adjustments of the code linearization. There is also a slight downside; where the previous index.js still reflected the order of the former monolithic source and of the categories in the documentation, I had to rethink that order in the new index.js. I think the new order makes sense in its own way, but it is a bit more fragmented. There is now a separate section for find-like functions and I had to isolate_.omit
from the other object functions, which I'm not entirely happy with. While writing this, I'm thinking that it might be slightly better to move_.pick
with it so at least they aren't so far apart. - I checked all sections in the source code for literate self-sufficiency. By this I mean I checked that (a) every piece of code is preceded by a comment, (b) no comment makes reference to comments or code outside of its own module and (c) every comment can be understood if you read its module in isolation. I made an exception on (a) for
isType
functions that purely consist of a call totagTester
, because I was able to group all of those directly after the definition oftagTester
itself and I think the monolithic read is more natural without comments between them.
I think the resulting single read is much improved compared to the previous review, although I'm afraid it's still not as beautiful as Underscore 0.4.0.
@jashkenas Please review again and let me know what you think!
modules/index.js
Outdated
export { default as find, default as detect } from './find.js'; | ||
export { default as findWhere } from './findWhere.js'; | ||
|
||
// Collection Functions (plus `omit`). |
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.
Likewise.
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: "Collection functions work on both arrays and objects" (and Maps and Sets if/when we choose to support them)
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 should have specified what I was unhappy about. I'm fine with the "Collection Functions" part but not with the "(plus omit
)" part. I felt compelled to append that because omit
is out of place here, but it feels ugly.
I do like your suggestions to clarify the section titles a bit, though. I'll follow up on them.
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.
From a quick read, it certainly looks like it passes the bar to me. There’s just the matter of the odd
// This module is the package entry point for ES module users. In other words,
comment towards the bottom of the file. It looks like it’s just grabbing the first line from index-all.js.
Glad to hear it passes the bar for you! Please see also this comment in case you missed it: #2849 (comment). Good catch about those comments. I think I'll embrace Rollup's "feature" of copying only the first line of a module's opening comment; if I rewrite that line so that it makes sense in isolation, it may help clarify what is going on at the bottom of the monolithic bundle. |
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.
@jashkenas I fixed the sections in the index.js
(I think) and addressed those lines that were ripped out of context at the bottom of the monolithic bundles. At this point, I'm quite confident that all modules as well as the monolithic bundles are at least as readable, clear and good-looking as the code on the master branch, but I'd like to ask your opinion one more time.
Please take a look at the revamped index.js
and my comments below. I hope you like it.
modules/_baseIteratee.js
Outdated
// element in a collection, returning the desired result — either `identity`, | ||
// element in a collection, returning the desired result — either `_.identity`, |
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.
Anywhere a comment references a public Underscore function, I adopted the convention of prefixing the name with _.
. Hopefully, this will make it easier to distinguish these names from references to other functions and local variables.
// Default Export | ||
// ============== |
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 gave index.js
, index-default.js
and index-all.js
a proper title. Besides introducing the modules, these titles also make sense when inserted in the monolithic build:
Lines 1763 to 1769 in 443bcc0
// Named Exports | |
var allExports = { | |
__proto__: null, | |
VERSION: VERSION, | |
restArguments: restArguments, |
Lines 1909 to 1924 in 443bcc0
mixin: mixin, | |
'default': _ | |
}; | |
// Default Export | |
// Add all of the Underscore functions to the wrapper object. | |
var _$1 = mixin(allExports); | |
// Legacy Node.js API. | |
_$1._ = _$1; | |
// ESM Exports | |
export default _$1; | |
export { VERSION, after, every as all, allKeys, some as any, extendOwn as assign, before, bind, bindAll, chain, chunk, clone, map as collect, compact, compose, constant, contains, countBy, create, debounce, defaults, defer, delay, find as detect, difference, rest as drop, each, _escape as escape, every, extend, extendOwn, filter, find, findIndex, findKey, findLastIndex, findWhere, first, flatten$1 as flatten, reduce as foldl, reduceRight as foldr, each as forEach, functions, groupBy, has$1 as has, first as head, identity, contains as include, contains as includes, indexBy, indexOf, initial, reduce as inject, intersection, invert, invoke, isArguments$1 as isArguments, isArray, isArrayBuffer, isBoolean, isDataView, isDate, isElement, isEmpty, isEqual, isError, isFinite$1 as isFinite, isFunction$1 as isFunction, isMap, isMatch, isNaN$1 as isNaN, isNull, isNumber, isObject, isRegExp, isSet, isString, isSymbol, isTypedArray$1 as isTypedArray, isUndefined, isWeakMap, isWeakSet, iteratee, keys, last, lastIndexOf, map, mapObject, matcher, matcher as matches, max, memoize, functions as methods, min, mixin, negate, noop, now, object, omit, once, pairs, partial, partition, pick, pluck, property, propertyOf, random, range, reduce, reduceRight, reject, rest, restArguments, result, sample, filter as select, shuffle, size, some, sortBy, sortedIndex, rest as tail, first as take, tap, template, templateSettings, throttle, times, toArray, _unescape as unescape, union, uniq, uniq as unique, uniqueId, unzip, values, where, without, wrap, zip }; | |
//# sourceMappingURL=underscore-esm.js.map |
The only remaining imperfection is that the sourceMappingURL
all the way at the bottom ends up being rendered as a level 1 heading by Docco. There's not much we can do about that, other than creating a modified version of the ESM bundle that excludes the link to the source map just for Docco.
Maybe give me a sense of what you’d like from me for another review? A read through and testing of the diff again? Or just a last minute thumbs up to recognize that we’re still on the same page? |
Just a thumbs-up, mostly for the |
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.
Great, thanks! Here’s a few copy tweaks if you want them...
Small words tweaks to index.js
Thanks a lot Jeremy! |
Boom, Underscore is completely modular from now on! I will work towards a new release next, but before that, I want to do a couple of things:
|
Hip hip hooray! Congratulations, @jgonggrijp! The longevity of open source meaningful, positive contribution continues to surprise and impress. |
After #2826, I'm sure some people have been waiting for this. Here it is, finally!
Enhancements
modules/index.js
is split into lots of tiny modules in order to promote code reuse and to facilitate people who want to create their own bundles with an even smaller Underscore footprint.modules/_setup.js
._
, e.g.,modules/_baseCreate.js
.modules/index.js
still exists, it is now a module that pulls in all the exports from the individual public function modules and exposes them as named exports with aliases. Its interface has not changed compared to the semimonolithic version.collectNonEnumProps
so that it doesn't depend on_.contains
anymore. See also compatibility notes below.underscore-esm.js
for the time being; see compatibility notes below.@jashkenas: both versions of the annotated source are slightly different from what I have shown you before, so you may want to re-read them.
Tradeoffs
import
statement. This is not a problem as long as that comment only makes sense when you can also see theimport
statement itself, but it does mean that developers should avoid writing important comments about actual logic before animport
statement.Compatibility notes
I have opted out of all breaking changes that may seem natural to combine with modularization. I did this because I want there to be a backwards-compatible modular 1.x release before we move on to a breaking Underscore 2.0 release. Having 1.x and 2.x branches that are structured in the same way makes it much easier to share bugfixes between them, so that we can continue to support the 1.x series for a while. It also simplifies development work on other libraries that may want to have separate support branches for Underscore 1.x and Underscore 2.x, such as (a revived) Underscore-Contrib.
So I have not made the following changes and I propose to make them in Underscore 2.0 instead:
underscore.js
tounderscore-umd.js
and the newunderscore-esm.js
tounderscore.js
.collectNonEnumProps
entirely to shake weight again (see also Drop support for IE < 11 #2325).modules/index.js
. There are three stages of “completeness” of the_
object and users should make a conscious choice as to which one they want to import, instead of ambiguously importing the middle one from the index (which may not be what they expect):modules/underscore.js
;modules/underscore-oop.js
;modules/index-default.js
).Open questions
npm run bundle
, including on every commit. This adds a delay of about a second. We may want to take a slightly more sophisticated approach to this. One of the options is to put them in a separate Rollup config so that we can separate building the monolithic bundles from building the modular AMD and CJS versions. I'm open to suggestions.package.json
are starting to get a bit unwieldly. It may be worth investigating a dedicated automation tool such as Gulp, although I think this would belong in a new pull request.ctor
inmodules/create.js
in order to reduce side effects. However, I'm not sure this is actually necessary. We may be able to shave off a few bytes by undoing that.Next steps
Edit to add: You can support my work on Underscore and other open source projects on Patreon.