v2.2.0 - The included `global.define` breaks UMD defs in other scripts #1544

Closed
marshallswain opened this Issue Mar 24, 2015 · 8 comments

Comments

Projects
None yet
3 participants
@marshallswain
Member

marshallswain commented Mar 24, 2015

In order to use can.jquery.dev.js in its current form, I have to delete window.define:

    <script src="/bower_components/jquery/dist/jquery.min.js"></script>
    <script src="/bower_components/canjs/can.jquery.dev.js"></script>
    <script src="/bower_components/canjs/can.stache.js"></script>
    <script src="/bower_components/canjs/can.map.define.js"></script>
    <script src="/bower_components/canjs/can.map.delegate.js"></script>
    <script src="/bower_components/canjs/can.list.promise.js"></script>
    <script src="/bower_components/canjs/can.list.sort.js"></script>
    <script src="/bower_components/canjs/can.fixture.js"></script>

    <script>delete window.define;</script>

    <script src="/socket.io/socket.io.js"></script>
    <script src="/bower_components/momentjs/min/moment.min.js"></script>
    <script src="/bower_components/accounting.js/accounting.min.js"></script>
    <script src="/bower_components/feathers-websocket-client/dist/feathers-websocket-client.js"></script>
    <script src="/bower_components/canjs-feathers/lib/feathers.js"></script>

If not, because can.jquery.dev.js includes the following lines:

// Line 26: 
var ourDefine = global.define = function(moduleName, deps, callback){
...
// Line 60: 
global.define.amd = true;

This breaks any UMD definitions in libraries loaded after CanJS:

(function (root, factory) {
  if (typeof define === 'function' && define.amd) {
      // AMD mode gets run instead of window global stuff, below.
      define(['can/util/can', 'feathers-websocket-client'], factory);
  } else if (typeof exports === 'object') {
      module.exports = factory(require('can/util/can'),  require('feathers-websocket-client'));
  } else if(typeof window !== 'undefined' && root && root.can && root.Feathers.client) {
      // This would run if define.amd wasn't true.
      factory(root.can, root.Feathers.client);
  }
}(this, function (can, feathersClient) {
@matthewp

This comment has been minimized.

Show comment
Hide comment
@matthewp

matthewp Mar 24, 2015

Contributor

I think we went with define as the name because of dojo. I don't think we can rename it but it should be deleting it itself.

Contributor

matthewp commented Mar 24, 2015

I think we went with define as the name because of dojo. I don't think we can rename it but it should be deleting it itself.

@marshallswain

This comment has been minimized.

Show comment
Hide comment
@marshallswain

marshallswain Mar 24, 2015

Member

It's for sure not deleting itself. The other CanJS plugins are dependent on it being in place. If I move the delete script up above any of the plugins, they fail with varying errors. If I move the delete script before loading can.fixture.js:

    <script src="/bower_components/jquery/dist/jquery.min.js"></script>
    <script src="/bower_components/canjs/can.jquery.dev.js"></script>
    <script src="/bower_components/canjs/can.stache.js"></script>
    <script src="/bower_components/canjs/can.map.define.js"></script>
    <script src="/bower_components/canjs/can.map.delegate.js"></script>
    <script src="/bower_components/canjs/can.list.promise.js"></script>
    <script src="/bower_components/canjs/can.list.sort.js"></script>

    <script>delete window.define;</script>

    <script src="/bower_components/canjs/can.fixture.js"></script>

I get the following error
screen shot 2015-03-23 at 6 30 17 pm

Member

marshallswain commented Mar 24, 2015

It's for sure not deleting itself. The other CanJS plugins are dependent on it being in place. If I move the delete script up above any of the plugins, they fail with varying errors. If I move the delete script before loading can.fixture.js:

    <script src="/bower_components/jquery/dist/jquery.min.js"></script>
    <script src="/bower_components/canjs/can.jquery.dev.js"></script>
    <script src="/bower_components/canjs/can.stache.js"></script>
    <script src="/bower_components/canjs/can.map.define.js"></script>
    <script src="/bower_components/canjs/can.map.delegate.js"></script>
    <script src="/bower_components/canjs/can.list.promise.js"></script>
    <script src="/bower_components/canjs/can.list.sort.js"></script>

    <script>delete window.define;</script>

    <script src="/bower_components/canjs/can.fixture.js"></script>

I get the following error
screen shot 2015-03-23 at 6 30 17 pm

@marshallswain

This comment has been minimized.

Show comment
Hide comment
@marshallswain

marshallswain Mar 24, 2015

Member

I don't understand why each plugin is dependent on it, when each contains the same /*[global-shim]*/ that creates the define.

Member

marshallswain commented Mar 24, 2015

I don't understand why each plugin is dependent on it, when each contains the same /*[global-shim]*/ that creates the define.

@marshallswain

This comment has been minimized.

Show comment
Hide comment
@marshallswain

marshallswain Mar 24, 2015

Member

Which is the better practice: loading scripts from a widely-used CDN or bundling them into a larger libs file, say jQuery and CanJS and MomentJS in one file? My guess would be that bundling would be best for creating an independent app, like a PhoneGap project. General web apps would probably benefit more from using the CDN, especially once HTTP2 is in place. Am I off base with my thinking?

Member

marshallswain commented Mar 24, 2015

Which is the better practice: loading scripts from a widely-used CDN or bundling them into a larger libs file, say jQuery and CanJS and MomentJS in one file? My guess would be that bundling would be best for creating an independent app, like a PhoneGap project. General web apps would probably benefit more from using the CDN, especially once HTTP2 is in place. Am I off base with my thinking?

@marshallswain

This comment has been minimized.

Show comment
Hide comment
@marshallswain

marshallswain Mar 24, 2015

Member

I don't understand why each plugin is dependent on it, when each contains the same /[global-shim]/ that creates the define.

Oh. Probably because it's now missing what appears to be a module cache in define.modules.

Member

marshallswain commented Mar 24, 2015

I don't understand why each plugin is dependent on it, when each contains the same /[global-shim]/ that creates the define.

Oh. Probably because it's now missing what appears to be a module cache in define.modules.

@matthewp

This comment has been minimized.

Show comment
Hide comment
@matthewp

matthewp Mar 24, 2015

Contributor

@marshallswain best practice is not to use the global build :) I think you might have a point though, deleting window.define might break things because it depends on define.modules, so we do need to cache modules somewhere.

Contributor

matthewp commented Mar 24, 2015

@marshallswain best practice is not to use the global build :) I think you might have a point though, deleting window.define might break things because it depends on define.modules, so we do need to cache modules somewhere.

@matthewp

This comment has been minimized.

Show comment
Hide comment
@matthewp

matthewp Mar 24, 2015

Contributor

Created stealjs/steal-tools#200 to discuss fixing this issue. I think we do need a global but maybe it can't be define.

Contributor

matthewp commented Mar 24, 2015

Created stealjs/steal-tools#200 to discuss fixing this issue. I think we do need a global but maybe it can't be define.

@daffl daffl added this to the 2.2.1 milestone Mar 24, 2015

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Mar 24, 2015

Contributor

This has been fixed via stealjs/steal-tools#201

Contributor

daffl commented Mar 24, 2015

This has been fixed via stealjs/steal-tools#201

@daffl daffl closed this Mar 24, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment