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

[RFC] umd shrinkage #36

Merged
merged 1 commit into from
Jan 6, 2015
Merged

[RFC] umd shrinkage #36

merged 1 commit into from
Jan 6, 2015

Conversation

leebyron
Copy link
Contributor

@leebyron leebyron commented Jan 6, 2015

This UMD definition is more terse. It's based on https://github.com/umdjs/umd/blob/master/returnExports.js but takes advantage of some expressions and implicit global scope for the fallback case.

( Need to update tests before this is pullable, wanted to get it up for comments first )

@leebyron
Copy link
Contributor Author

leebyron commented Jan 6, 2015

Example before after for a simple module with no dependencies (strict umd):

// Before

(function (global, factory) {

  'use strict';

  if (typeof define === 'function' && define.amd) {
    // export as AMD
    define('myModule', [], factory);
  } else if (typeof module !== 'undefined' && module.exports && typeof require === 'function') {
    // node/browserify
    factory();
  } else {
    // browser global
    global.myModule = {};
    factory();
  }

}(typeof window !== 'undefined' ? window : this, function () {

  'use strict';

  console.log( 'hello' );

}));

// After

(function (factory) {
  typeof define === 'function' && define.amd ?
    define('myModule', [], factory) :
  typeof exports === 'object' ?
    factory() :
  (myModule = {}, factory());
}(function () {
  'use strict';

  console.log( 'hello' );

}));

@Rich-Harris
Copy link
Contributor

Looks good - the current header is a bit on the long side. Occurs to me we could even take this further and differentiate between modules/bundles that have imports/exports and those that don't - for instance the example above could be more compact (at the same time, it would make sense to drop the name requirement for UMD with no exports, though that's a separate issue).

(function (factory) {
  typeof define === 'function' && define.amd ?
    define('myModule', [], factory) :
    factory();
}(function () {
  'use strict';

  console.log( 'hello' );
}));

Possibly overkill considering how rare that situation is though.

This does mean we're introducing a strict mode violation (myModule = {} rather than global.myModule = {}) - do we care about that?

Btw, to save you some manual labour, once the output is as you expect, you can generate test results automatically with node test/utils/generate.js.

@eventualbuddha
Copy link
Contributor

I would be a little concerned about loosening up the CommonJS environment check too much. Checking for just exports or module has bit me before. I've never explicitly looked for the require function though, and I wonder if that would fail in the modern browserify (becomes _dereq or something, or would browserify rewrite it despite it not being a call?).

The ternary expression transformation is something that a good minifier should be able to do, though it doesn't hurt to help it along. Overall I like the direction of us, and have been thinking about a way to simply bundle the files together without any sort of environment checks by esperanto (essentially allow custom handling).

@leebyron
Copy link
Contributor Author

leebyron commented Jan 6, 2015

This does mean we're introducing a strict mode violation (myModule = {} rather than global.myModule = {}) - do we care about that?

I don't think so, but I think I will revert this. I realized later that this could create more work for minifiers and it's probably not worth the trouble.

I would be a little concerned about loosening up the CommonJS environment check too much. Checking for just exports or module has bit me before.

Yes, I was also worried about this as well and went about searching for UMD wrappers. I based it on umdjs which has a lot of usage and hasn't seemed to take any heat for this. Also, immutable.js has a custom UMD wrapper which uses this check and I've gotten no complaints.

I think most don't check for require since functions by the same name have been overused. define

The ternary expression transformation is something that a good minifier should be able to do.

I so agree. Unfortunately I haven't found one that will be this bold yet.

This UMD definition is more terse. It's based on https://github.com/umdjs/umd/blob/master/returnExports.js but takes advantage of some expressions and implicit global scope for the fallback case.
@leebyron
Copy link
Contributor Author

leebyron commented Jan 6, 2015

Occurs to me we could even take this further and differentiate between modules/bundles that have imports/exports and those that don't

Nice idea! I updated to include that case. Also updated all tests.

If you guys are happy with this, it should be in a mergeable state.

@Rich-Harris Rich-Harris merged commit 326f89c into esperantojs:master Jan 6, 2015
@Rich-Harris
Copy link
Contributor

Just realised this should apply to one-to-one conversions too. Working on it

@leebyron
Copy link
Contributor Author

leebyron commented Jan 6, 2015

Ah! Sorry I forgot about those.

@Rich-Harris
Copy link
Contributor

NP! I missed it too. Have pushed out a new release.

@eventualbuddha

have been thinking about a way to simply bundle the files together without any sort of environment checks by esperanto (essentially allow custom handling)

Definitely interested to hear your thoughts on custom handling. I keep thinking 'surely it's straightforward' but then discover some quirk of how AMD and CJS differ that makes it a pain to generalise, but I'm probably just not being imaginative enough. A plain bundle would be straightforward though, as long as there's no external dependencies (or you can safely assume they're on the page already)

@leebyron
Copy link
Contributor Author

leebyron commented Jan 6, 2015

a "raw strict" mode might be nice. The top-level module exports via exports.foo and the whole package is in the top scope, no wrapping closure.

@fcarstens
Copy link

This shortened definition caused problems for us, because we had some other code defining module and exports. I think you shouldn't omit the module.exports check from the original header.

refs qunitjs/js-reporters#22

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