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

remove jquery from ember pkg #16130

Closed
wants to merge 3 commits into from
Closed

Conversation

snewcomer
Copy link
Contributor

@snewcomer snewcomer commented Jan 16, 2018

Down to a few tests.

  • nested routes and link-to arguments: The {{link-to}} helper supports bubbles=false
  • nested routes and link-to arguments: The {{link-to}} helper supports bubbles=boundFalseyThing

Something is happening with the click handler

References #16058

@snewcomer snewcomer force-pushed the ember-no-jquery branch 2 times, most recently from 56a1bca to c9483d7 Compare January 16, 2018 05:48
<div id="one"></div>
<div id="two"></div>
`);
document.getElementById('qunit-fixture').innerHTML =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a specialized system in place for this (added in ff9117a). You should be able to do:

class extends ApplicationTestCase {
  get fixture() {
    return `<div id="one"></div><div id="two"></div>`;
  }
}

@snewcomer
Copy link
Contributor Author

@rwjblue Looking at the link_to_test, with jQuery off, do you happen to know if anything changes wrt bubbles=false? Passes w/ jQuery, but fails w/o jQuery. And the only change is to a find selector that passes.

@rwjblue
Copy link
Member

rwjblue commented Jan 16, 2018

do you happen to know if anything changes wrt bubbles=false?

No, I can't think of one off the top of my head...

@snewcomer
Copy link
Contributor Author

@rwjblue looks like this line is undefined so it jumps down to the parentNode and passes that to the eventListener.

https://github.com/emberjs/ember.js/blob/master/packages/ember-views/lib/system/event_dispatcher.js#L299

Starts here in the view_support mixin, eventually calling down into ember-metal.

https://github.com/emberjs/ember.js/blob/master/packages/ember-views/lib/mixins/view_support.js#L456

Failing to figure out the exact problem. Seems at this point it may be test setup since I see other tests not using jquery and handling bubbling correctly.

@ro0gr
Copy link
Contributor

ro0gr commented Jan 16, 2018

@snewcomer I've experienced a similar issue when I've been removing jquery from the ember-glimmer package. The fix was to return bubble value from the action handler.

I've made a pr against your branch with a similar fix (snewcomer#1)
@rwjblue can you confirm if that is a proper fix?

@ro0gr
Copy link
Contributor

ro0gr commented Jan 16, 2018

I've made a pr

This change broke 2 other tests:

  • EventDispatcher: events bubbling up can be prevented
  • View tree tests: getChildViews

No matter with or w/o jquery. I've closed the pr. Sorry for the false positive 😫

@ro0gr
Copy link
Contributor

ro0gr commented Jan 16, 2018

@snewcomer @rwjblue I believe this is a more correct approach snewcomer#2

@rwjblue
Copy link
Member

rwjblue commented Jan 16, 2018

Hmm, @ro0gr I was thinking the issue was regarding this return value in LinkComponent#_invoke (roughly similar to the fix here ad6bc61).

I think that return from _invoke should become:

let shouldBubble = get(this, 'bubbles') !== false;
if (!shouldBubble) { event.stopPropagation(); }
// ...snip...
return shouldBubble;

@ro0gr
Copy link
Contributor

ro0gr commented Jan 16, 2018

@rwjblue unfortunately changing of _invoke to return a proper bubbling value doesn't fix 2 link-to tests from the description.

As far as I can see _invoke is only used as an event handler. I think it makes impossible to consume _invoke's return value.

@ro0gr
Copy link
Contributor

ro0gr commented Jan 19, 2018

So I'm out of ideas on how to fix link-to's bubbles behavior.

@rwjblue @snewcomer What do you think about skipping these 2 link-to tests in non-jquery mode for this last package?

@snewcomer
Copy link
Contributor Author

@rwjblue I looked at a few PR's @ro0gr put up and I still can't figure out a sol'n. Any further ideas?

@snewcomer
Copy link
Contributor Author

closing for now.

@snewcomer snewcomer closed this May 28, 2018
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 this pull request may close these issues.

3 participants