ember-cli / ember-compatibility-helpers

Helpers that allow you to write backwards compat Ember addons
MIT License
24 stars 20 forks source link

In addon: only runs in the dummy app #32

Closed ppcano closed 5 years ago

ppcano commented 5 years ago

https://github.com/ppcano/compatibility-helper-addon reproduces the error.

1) In the dummy app, using ember-compatibility-helpers works and Babel transforms properly the flag of the dummy.js file.

// tests/dummy/app/controllers/application.js

import { gte } from "ember-compatibility-helpers";
import Controller from "@ember/controller";
import { computed } from "@ember/object";

export default Controller.extend({
  gte3: computed(function() {
    return gte("3.0.0");
  })
});

2) In the addon folder, it triggers the error Uncaught Error: Could not find module ember-compatibility-helpers imported from compatibility-helper-addon/components/test-component

// addon/components/test-component.js

import Component from '@ember/component';
import layout from '../templates/components/test-component';
import { computed } from "@ember/object";

import { gte } from "ember-compatibility-helpers";

export default Component.extend({
  layout,
  gte3: computed(function() {
    return gte("3.0.0");
  })
});

This behaviour looks to be totally different than the one reported at #29, the issue was initially reported at https://github.com/ember-cli/ember-welcome-page/pull/124

pzuraq commented 5 years ago

This is a common issue, you need to move it to your addon’s dependencies instead of devDeps. I wish there was a way to warn people about this, this is true of all addons and I see people run into it all the time 😩

ppcano commented 5 years ago

@pzuraq You were right with this example, adding the addon to the dependencies solved the issue, however, 🤪🤯 this does not look to solve the issue on the ember-welcome-page example (the one that I initially found).

The error is reproduced at https://github.com/ember-cli/ember-welcome-page/pull/127 (CI job shows the commented error)

https://github.com/ember-cli/ember-welcome-page/pull/127/commits/0c6ec853162aa5eb052bb8491043b2f1cfe6d015

pzuraq commented 5 years ago

I believe the issue comes from this line here: https://github.com/ember-cli/ember-welcome-page/pull/127/files#diff-168726dbe96b3ce427e7fedce31bb0bcR8

This is another one of those gotchas that I wish we could teach better. Basically, calling this._super.included(app) binds this to this._super, the addon's parent class, instead of the addon itself. This is bad news bears 😞

You have to do this instead to bind this correctly:

this._super.included.call(this, app);

// OR
this._super.included.apply(this, arguments);
ppcano commented 5 years ago

👏this is the issue (I would have never found it).

The guide is documented correctly with:

this._super.included.apply(this, arguments);

@pzuraq Thank you!

pzuraq commented 5 years ago

It is super counterintuitive - I still regularly do this when setting up a new addon. Hopefully we can fix it in the future!

rwjblue commented 5 years ago

yeah, its super trolly. we should file issues with all of these addons (533!?!?!?!)

https://emberobserver.com/code-search?codeQuery=this._super.included(

pzuraq commented 5 years ago

Any chance we can just fix it CLI by binding the functions on _super? Should be backwards compatible I think.

The reason it’s so trolly is it works if you do it wrong, until you have subaddons. Could probably start warning if this is bound incorrectly?

ppcano commented 5 years ago

I think it may be good to open an issue on the ember-cli repo to avoid forgetting about it (I don't know all the details to describe it accurately).