Fix issue with app.import being undefined

[sandersky]

This fixes the following error I'm coming across in an application that consumers the addon ember-prop-types which enables the includePolyfill flag here.

app.import is not a function
TypeError: app.import is not a function
  at Class.module.exports.importPolyfill (/Repositories/my-app/node_modules/ember-cli-babel/index.js:35:15)
  at Class.module.exports.included (/Repositories/my-app/node_modules/ember-cli-babel/index.js:60:12)
  at /Repositories/my-app/node_modules/ember-cli/lib/models/addon.js:244:32
  at Array.map (native)
  at Class.eachAddonInvoke (/Repositories/my-app/node_modules/ember-cli/lib/models/addon.js:242:22)
  at Class.Addon.included (/Repositories/my-app/node_modules/ember-cli/lib/models/addon.js:349:8)
  at EmberApp.<anonymous> (/Repositories/my-app/node_modules/ember-cli/lib/broccoli/ember-app.js:433:15)
  at Array.filter (native)
  at EmberApp._notifyAddonIncluded (/Repositories/my-app/node_modules/ember-cli/lib/broccoli/ember-app.js:428:45)
  at new EmberApp (/Repositories/my-app/node_modules/ember-cli/lib/broccoli/ember-app.js:109:8)
  at module.exports.defaults (/Repositories/my-app/ember-cli-build.js:6:13)
  at Class.module.exports.Task.extend.setupBroccoliBuilder (/Repositories/my-app/node_modules/ember-cli/lib/models/builder.js:55:19)
  at Class.module.exports.Task.extend.init (/Repositories/my-app/node_modules/ember-cli/lib/models/builder.js:89:10)
  at new Class (/Repositories/my-app/node_modules/ember-cli/node_modules/core-object/core-object.js:18:12)
  at Class.module.exports.Task.extend.run (/Repositories/my-app/node_modules/ember-cli/lib/tasks/build.js:15:19)
  at Class.<anonymous> (/Repositories/my-app/node_modules/ember-cli/lib/commands/test.js:162:27)
  at lib$rsvp$$internal$$tryCatch (/Repositories/my-app/node_modules/rsvp/dist/rsvp.js:1036:16)
  at lib$rsvp$$internal$$invokeCallback (/Repositories/my-app/node_modules/rsvp/dist/rsvp.js:1048:17)
  at /Repositories/my-app/node_modules/rsvp/dist/rsvp.js:331:11
  at lib$rsvp$asap$$flush (/Repositories/my-app/node_modules/rsvp/dist/rsvp.js:1198:9)
  at doNTCallback0 (node.js:428:9)
  at process._tickCallback (node.js:357:13)

[sandersky]

@stefanpenner @Turbo87 would love to get this reviewed/merged as soon as possible as I suspect it will be breaking a bunch of apps with my latest changes to ember-prop-types. If I get feedback soon and it doesn't seem like the correct fix I'll simply undo support for PropTypes.symbol in ember-prop-types for now as to unblock this issue from downstream applications. Thanks

[rwjblue]

This should use this.import when present. That would enable this to work properly for nested addons (like ember-prop-types), whereas the guard simply makes it do nothing when used as a nested addon.

Something like:

if (this.import) {  // support for ember-cli >= 2.7
  this.import('vendor/browser-polyfill.js', { prepend: true });
} else { // support ember-cli < 2.7
  app.import('vendor/browser-polyfill.js', { prepend: true });
}

[sandersky]

Thanks for the clarification, I've applied your recommendation.

[rwjblue]

@sandersky - Can you confirm this fixes the issues you have identified upstream?

[sandersky]

@rwjblue it looks like I'm still hitting a case where app.import is undefined. I added the following for debugging purposes:

  importPolyfill: function(app) {
    if (this.import) {  // support for ember-cli >= 2.7
      this.import('vendor/browser-polyfill.js', { prepend: true });
    } else { // support ember-cli < 2.7
      console.info(Object.keys(app))
      app.import('vendor/browser-polyfill.js', { prepend: true });
    }
  },

which provided the following:

[
  'parent',
  'project',
  'ui',
  'addonPackages',
  'addons',
  'addonDiscovery',
  'addonsFactory',
  'registry',
  '_didRequiredBuildPackages',
  'nodeModulesPath',
  'treePaths',
  'treeForMethods',
  '_addonsInitialized',
  'options'
]

So it appears the app argument is being passed in but doesn't always include the import method.

[sandersky]

The following seems to fix it though:

  importPolyfill: function(app) {
    if (this.import) {  // support for ember-cli >= 2.7
      this.import('vendor/browser-polyfill.js', { prepend: true });
    } else { // support ember-cli < 2.7
      while (app.app) {
        app = app.app
      }

      if (app.import) {
        app.import('vendor/browser-polyfill.js', { prepend: true });
      }
    }
  },

Note: I hit another case where the app argument has the property app.

[rwjblue]

@sandersky - What version of ember-cli are you testing with? For ember-cli >= 2.7.0 this.import should work. Are you saying that it doesn't?

[sandersky]

@rwjblue I'm using ember-cli version 2.5.1 in the app where I'm seeing the issue (can't upgrade to the latest yet as I need to address issues in other downstream addons first).

[rwjblue]

I do not think we should add the "walk up" strategy all over the place. We fixed this issue in 2.7 with this.import. If app.import is not present here, I would be fine with a warning or something suggesting that the user manually add includePolyfill to their app.

[sandersky]

@rwjblue so are you suggesting something along the lines of:

  importPolyfill: function(app) {
    if (this.import) {  // support for ember-cli >= 2.7
      this.import('vendor/browser-polyfill.js', { prepend: true });
    } else if (app.import) { // support ember-cli < 2.7
      app.import('vendor/browser-polyfill.js', { prepend: true });
    } else {
      console.warn('Please add includePolyfill to your ember-cli-babel config')
    }
  },

My only concern is you get the warning even after you've added the includePolyfill option to the apps babel configuration.

[rwjblue]

Perhaps we could suggest they install https://github.com/ember-cli/ember-cli-import-polyfill into the app instead? That would prevent the warning on subsequent runs...

[sandersky]

@rwjblue I like that idea. I just verified it works as desired and committed the changes. If there are no more concerns I'm 100% satisfied with the latest approach. Thanks for the feedback/advice.

[rwjblue]

This looks good to me. Thanks for working through the various edge cases with me.

@Turbo87 / @stefanpenner - One of y'all will have to do the honors of merging + releasing as I don't have GH permissions here yet.

[Turbo87]

@rwjblue if I understood @ef4 correctly then the import polyfill should be installed in this repository here and not in any app

[rwjblue]

I believe that you have recalled it backwards. ember-cli/ember-cli-import-polyfill#1 (comment)

The polyfill is intended for apps with ember-cli older than 2.7.

The whole point of the polyfill was making it so we don't need to make changes in a lot of addons. Addons do not need the polyfill. Addon authors should be telling users to upgrade to ember-cli 2.7 or install the polyfill.

[Turbo87]

riiiiight.... 🙊

[Turbo87]

@sandersky thanks for working through this!

[Turbo87]

published as v5.1.10

[sandersky]

Thanks for getting this merged so quickly @Turbo87 it appears to address the issues I was having.