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

Fix issue with app.import being undefined #89

Merged
merged 4 commits into from Aug 15, 2016

Conversation

Projects
None yet
3 participants
@sandersky
Copy link
Contributor

sandersky commented Aug 15, 2016

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

This comment has been minimized.

Copy link
Contributor Author

sandersky commented Aug 15, 2016

@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

@@ -31,7 +31,9 @@ module.exports = {
},

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

This comment has been minimized.

@rwjblue

rwjblue Aug 15, 2016

Member

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 });
}

This comment has been minimized.

@sandersky

sandersky Aug 15, 2016

Author Contributor

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

@rwjblue

This comment has been minimized.

Copy link
Member

rwjblue commented Aug 15, 2016

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

@sandersky

This comment has been minimized.

Copy link
Contributor Author

sandersky commented Aug 15, 2016

@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

This comment has been minimized.

Copy link
Contributor Author

sandersky commented Aug 15, 2016

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

This comment has been minimized.

Copy link
Member

rwjblue commented Aug 15, 2016

@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

This comment has been minimized.

Copy link
Contributor Author

sandersky commented Aug 15, 2016

@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

This comment has been minimized.

Copy link
Member

rwjblue commented Aug 15, 2016

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

This comment has been minimized.

Copy link
Contributor Author

sandersky commented Aug 15, 2016

@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

This comment has been minimized.

Copy link
Member

rwjblue commented Aug 15, 2016

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

This comment has been minimized.

Copy link
Contributor Author

sandersky commented Aug 15, 2016

@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 comment has been minimized.

Copy link
Member

rwjblue commented Aug 15, 2016

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

This comment has been minimized.

Copy link
Contributor

Turbo87 commented Aug 15, 2016

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

@rwjblue

This comment has been minimized.

Copy link
Member

rwjblue commented Aug 15, 2016

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

This comment has been minimized.

Copy link
Contributor

Turbo87 commented Aug 15, 2016

riiiiight.... 🙊

@Turbo87 Turbo87 merged commit 0c8aec5 into babel:master Aug 15, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Turbo87

This comment has been minimized.

Copy link
Contributor

Turbo87 commented Aug 15, 2016

@sandersky thanks for working through this!

@Turbo87

This comment has been minimized.

Copy link
Contributor

Turbo87 commented Aug 15, 2016

published as v5.1.10

@sandersky

This comment has been minimized.

Copy link
Contributor Author

sandersky commented Aug 16, 2016

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

@Turbo87 Turbo87 added the enhancement label Dec 5, 2016

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