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

AMD -> ES6 modules #19

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

Rich-Harris
Copy link
Contributor

As discussed via email, this PR converts all the AMD modules into their ES module equivalents. It's probably not ready to merge in its current state (e.g. I haven't updated the tests, though I have updated all the examples and they still work with one exception, more on which below), but it's a starting point for the sake of discussion.

Benefits:

  • Smaller, far more readable UMD bundle
  • Future-proof
  • Nicer format to author in (one less level of indentation!)
  • Tree-shakeability (i.e. zero-config custom builds for consumers of the library using ES6-aware tooling like Rollup or Webpack 2)

It doesn't introduce any ES6 features other than import and export, so there's still no need to transpile the code with Babel, if that's not desirable. The src folder was generated with convert-samsara-to-es, so that it can be re-run following any future changes.

Resulting dist file is here (compare to original).

The only theoretical downside is that AMD users can no longer import an individual module directly from the source, but instead require the entire UMD bundle. I'd argue that it's not such a loss – most of the examples end up depending on almost every module anyway, so they fetch almost as many bytes but spread over several dozen requests, and in any case if you were to adopt other ES6 language features as per #18 then you'd lose the build-free benefit of AMD anyway. That being said, it would be fairly trivial to create a directory of AMD modules from the ES module source at publish time, if necessary.

As well as the UMD build, this PR introduces a jsnext:main build, which is identical to the UMD build except that it uses the export statement rather than supporting CommonJS, AMD and globals. This is very beneficial for consumers of the library who are using ES6-aware tooling, as it allows any unused classes etc to be tree-shaken off. jsnext:main is a convention that's been adopted by a few major libraries (D3, Redux, lodash etc). One consequence of distributing an ES module is that a library can't have nested namespaces (like Samsara.DOM) – instead, everything exists in the top-level namespace:

// this...
import { Context, MouseInput, Timer } from 'samsarajs';

// ...rather than this
var Samsara = require( 'samsarajs' );
var Context = Samsara.DOM.Context;
var MouseInput = Samsara.Inputs.MouseInput;
var Timer = Samsara.Core.Timer;

// ...or this
var Context = require( 'samsara/dom/Context' );
var MouseInput = require( 'samsara/core/MouseInput' );
var Timer = require( 'samsara/core/Timer' );

In my view this might actually be better, because it's no longer necessary to remember whether Scrollview belongs to the DOM namespace or Layouts (for example), which makes learning and remembering the library much easier, and you have more freedom to alter the project structure without breaking people's apps. But if you think this is a change too far it's easy enough to maintain the current nested namespaces (it's just less future-proof, because it goes against the grain of ES modules).

There is one odd thing I've just noticed: the SideMenu demo no longer registers the drag gesture. No error is reported. Possibly an order-of-execution thing – will keep prodding it and see if I can work out what happened. Everything else appears to work identically. fixed by #20

Anyway, thanks for reading this far, this was a far more rambly PR than I planned 😅

@dmvaldman
Copy link
Owner

Thanks for putting the work into this! I'll need to consider these ramifications carefully and experiment with this workflow, but I'm glad you made it easy for me to test out. Will get back with my thoughts soon.

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.

2 participants