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

fix(glimmer-wrapper): add normalize method for Registry#has #300

Merged
merged 2 commits into from Dec 25, 2018

Conversation

buschtoens
Copy link
Contributor

Fixes #299.

@buschtoens
Copy link
Contributor Author

@mixonic #195 (comment):

Doing this in Ember's current service injection system is tricky since the injections are part of metal but dasherize etc are not. I punted and simply implemented in this repo for now.

tbh, I think we should still try to land that fix upstream. Doing it in here is really brittle.

keyName would need dasherization here: https://github.com/emberjs/ember.js/blob/f96a8df934fc8b4178f587b1cce609df15930922/packages/%40ember/-internals/metal/lib/injected_property.ts#L70

@rwjblue rwjblue merged commit 2a2b766 into ember-cli:master Dec 25, 2018
@lolmaus
Copy link

lolmaus commented Dec 26, 2018

🙇

@buschtoens
Copy link
Contributor Author

This seems to be broken. Resolver#normalize is null even though the following is included in the build:

  if (true) {
    Resolver.prototype.normalize = function (specifier) {
      // This method is called by `Registry#validateInjections` in dev mode.
      // https://github.com/ember-cli/ember-resolver/issues/299
      const [type, name] = specifier.split(':', 2);
      if (name && (type === 'service' || type === 'controller')) {
        return `${type}:${Ember.String.dasherize(name)}`;
      }
      return specifier;
    };
  }

I assume that this is because Ember manually collapses the prototype and normalize: null somehow takes precedence.

What is your suggested solution? Use .reopen({ ... }) or use a ternary in the original .extend({ ... }) block? I'm leaning towards a ternary like:

const Resolver = GlobalsResolver.extend({
  // ...

  normalize: !DEBUG ? null : function(specifier) {
    // This method is called by `Registry#validateInjections` in dev mode.
    // https://github.com/ember-cli/ember-resolver/issues/299
    const [type, name] = specifier.split(':', 2);
    if (name && (type === 'service' || type === 'controller')) {
      return `${type}:${dasherize(name)}`;
    }
    return specifier;
  },

  // ...
});

I assume constant ternaries are also optimized away by ember-cli-uglify / terser?

@buschtoens
Copy link
Contributor Author

Just tested the above fix in our app. It works. I'll send another PR.

buschtoens added a commit to buschtoens/ember-resolver that referenced this pull request Jan 9, 2019
@buschtoens buschtoens deleted the fix/299-normalize-for-has branch January 9, 2019 17:41
@rwjblue rwjblue added the bug label Feb 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants