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

Class constructor EmberError cannot be invoked without 'new' #5752

Closed
ssutar opened this issue Nov 16, 2018 · 9 comments
Closed

Class constructor EmberError cannot be invoked without 'new' #5752

ssutar opened this issue Nov 16, 2018 · 9 comments

Comments

@ssutar
Copy link

ssutar commented Nov 16, 2018

When used with ember-native-class-polyfill for ES6 classes, following error is thrown:

Uncaught TypeError: Class constructor EmberError cannot be invoked without 'new'
    at new AdapterError (vendor.js:132445)
    at Class.handleResponse (vendor.js:144384)
    at Class.handleResponse (vendor.js:122856)
    at Class.superWrapper (vendor-static.js:51234)
    at Class.handleResponse (vendor.js:89895)
    at Class.superWrapper [as handleResponse] (vendor-static.js:51234)
    at Class.handleResponse (vendor.js:119158)
    at Class.superWrapper [as handleResponse] (vendor-static.js:51234)
    at ajaxError (vendor.js:144655)
    at ajaxErrorHandler (vendor.js:144682)

It is originated from https://github.com/emberjs/data/blob/master/addon/-private/adapters/errors.js#L79

export function AdapterError(errors, message = 'Adapter operation failed') {
  this.isAdapterError = true;
  EmberError.call(this, message);

  this.errors = errors || [
    {
      title: 'Adapter Error',
      detail: message,
    },
  ];
}

As EmberError is a class with ember-native-class-polyfill, it cannot be called without new.

@ssutar
Copy link
Author

ssutar commented Nov 16, 2018

I have a fix at https://github.com/ssutar/data/blob/v_3_5_beta/addon/-private/adapters/errors.js#L79 but its not same as the current code.

Its something like this:

export function AdapterError(errors, message = 'Adapter operation failed') {
  this.isAdapterError = true;
  this.message = message;

  this.errors = errors || [
    {
      title: 'Adapter Error',
      detail: message,
    },
  ];
}

Its creating a new property message on the AdapterError instead of using one from EmberError. It works correctly due to prototype property lookup, but not sure if it is the correct fix.

cc: @rwjblue @krisselden

@pzuraq
Copy link

pzuraq commented Nov 19, 2018

This depends on usage, but I think this is meant to be a class, and if so the accurate update would be:

export class AdapterError extends EmberError {
  constructor(errors, message = 'Adapter operation failed') {
    super(message);
    this.isAdapterError = true;

    this.errors = errors || [
      {
        title: 'Adapter Error',
        detail: message,
      },
    ];
  }
}

@ssutar
Copy link
Author

ssutar commented Nov 19, 2018

Yeap, but would that need ember-native-class-polyfill to be included? 🤔That is the reason why I was not changing it to class. But let me try doing it, if tests pass I think we should be okay!

@pzuraq
Copy link

pzuraq commented Nov 19, 2018

Because this is a class which does not extend from EmberObject it should be fine. Native classes have worked by themselves for some time, just not when mixing with the Ember Object model.

@runspired
Copy link
Contributor

@pzuraq I read through the conversation about making Ember.Error extend from Error. Personally I would prefer to just eliminate use of Ember.Error here entirely and also extend from Error ourselves if possible.

@pzuraq
Copy link

pzuraq commented Nov 20, 2018

@runspired EmberError already extends from native Error, sort of. The fix in Ember would be to get rid of EmberError entirely, and just export native Error in its place:

emberjs/ember.js#17216

If we went this direction it would make more sense to remove AdapterError entirely, and instead rely on error codes or messages

@rwjblue
Copy link
Member

rwjblue commented Nov 20, 2018

Agree with @pzuraq, error subclasses are hard :(

@runspired
Copy link
Contributor

runspired commented Nov 20, 2018

@rwjblue @pzuraq am I interpreting this correctly to mean the subclassing in #5753 is not cross-browser #5753 ?

ssutar pushed a commit to ssutar/data that referenced this issue Nov 26, 2018
ssutar pushed a commit to ssutar/data that referenced this issue Nov 26, 2018
ssutar pushed a commit to ssutar/data that referenced this issue Nov 26, 2018
@runspired
Copy link
Contributor

We resolved this by reverting the change from function to class based definition of EmberError upstream.

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 a pull request may close this issue.

4 participants