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

[Bug] Observers don't work if the key contains colon ':' characters #19343

Closed
apellerano-pw opened this issue Jan 19, 2021 · 1 comment · Fixed by #19436
Closed

[Bug] Observers don't work if the key contains colon ':' characters #19343

apellerano-pw opened this issue Jan 19, 2021 · 1 comment · Fixed by #19436

Comments

@apellerano-pw
Copy link

🐞 Describe the Bug

Observers internally use colon in the eventName and assume there is only one colon when a split(':') operation is performed in activateObserver. By registering an observer with a key name that contains colons, you can break Ember's assumption.

🔬 Minimal Reproduction

emberObj.addObserver('key:with:colons', () => {
  console.log('I will never fire');
});

emberObj.set('key:with:colons', 'foo');

😕 Actual Behavior

The callback method never fires

🤔 Expected Behavior

The callback method fires

🌍 Environment

  • Ember: - 3.16.10
  • Node.js/npm: - node 10.23.0, npm 6.13.7
  • OS: - mac mojave 10.14.6
  • Browser: - chrome 87.0.4280.141

➕ Additional Context

The issue appears to be in the activateObserver function

  function activateObserver(target, eventName, sync = false) {
    var activeObservers = getOrCreateActiveObserversFor(target, sync);

    if (activeObservers.has(eventName)) {
      activeObservers.get(eventName).count++;
    } else {
      // the following line assumes there are no colons in the user-specified portion of the eventName
      var [path] = eventName.split(':');
      var tag = (0, _reference.combine)(getChainTagsForKey(target, path));
      activeObservers.set(eventName, {
        count: 1,
        path,
        tag,
        lastRevision: (0, _reference.value)(tag),
        suspended: false
      });
    }
  }

One gross fix would be something like

// split the string on colon, drop the final value, re-join on colon
var path = eventName.split(':').slice(0, -1).join(':')

But someone with a deeper understanding of the observer code may know of something more elegant

@rwjblue
Copy link
Member

rwjblue commented Jan 20, 2021

I think we can probably avoid splitting all together and use last index of + substr.

@pzuraq / @krisselden - what do you think?

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.

2 participants