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: Observer> trouble with observe Array (losted special methods) #9135

Closed
wants to merge 3 commits into from

Conversation

cybermerlin
Copy link

@cybermerlin cybermerlin commented Dec 3, 2018

What kind of change does this PR introduce? (check at least one)

  • Bugfix -------->> and for 2.5.17 in branch hotfix_observe_array
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

@windyflying
Copy link

@cybermerlin arrayMethods is an instance of Array.prototype. We redefined some methods of arrayMethods (such as push,splice,unshift ,etc.) in order to notify change.

@cybermerlin
Copy link
Author

@windyflying thx, I lost it before, but now fix =)

@cybermerlin
Copy link
Author

same decision in PR #9136

@posva
Copy link
Member

posva commented Dec 3, 2018

Repeating myself:

I feel like you have rushed this pr. If it's implementing that feature it would require proper testing of the actual feature request. Opening a pr with just Hotfix as a title is so
Confusing for us maintainers...

This one isn't clear at all either

You want to contribute to Open Source, that's great, but don't try to rush it like a feature on Friday evening. We need a runnable repro as @Justineo said for bug fixes

@cybermerlin
Copy link
Author

somebody help me with this #9137

@cybermerlin cybermerlin deleted the dev branch May 23, 2019 11:00
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