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

ns.ViewCollection баг обновления ноды вида #590

Open
chestozo opened this issue Apr 26, 2016 · 6 comments
Open

ns.ViewCollection баг обновления ноды вида #590

chestozo opened this issue Apr 26, 2016 · 6 comments

Comments

@chestozo
Copy link
Member

Вот тут:
https://github.com/yandex-ui/noscript/blob/master/src/ns.viewCollection.js#L671
ns.replaceNode(view._extractNode(newNode), view.node);

но если посмотреть на ns.replaceNode https://github.com/yandex-ui/noscript/blob/master/src/ns.dom.js#L9:

ns.replaceNode = function(oldNode, newNode) {
    // такая вот защита от лишних действий
    if (oldNode === newNode) {
        return true;
    }

    // если oldNode детачена из DOM, то у нее нет родителя
    if (oldNode.parentNode) {
        oldNode.parentNode.replaceChild(newNode, oldNode);
        return true;
    }

    return false;
};

Ну т.е. по ощущениям должно быть наоборот:
ns.replaceNode(view.node, view._extractNode(newNode));

Я прав / не прав?

@chestozo
Copy link
Member Author

После фикса тесты не упали, плохо покрыли :/

@eljusto
Copy link

eljusto commented Apr 26, 2016

Судя по коду, да https://github.com/yandex-ui/noscript/search?utf8=✓&q=ns.replaceNode
В других местах порядок аргументов правильный.

@chestozo
Copy link
Member Author

chestozo commented Apr 26, 2016

Подебажил на тестовом проекте - вроде бы всё корректно работает в текущей реализации https://github.com/chestozo/noscript-demo.
У нас по логам есть кейс, когда view._extractNode(newNode) ничего не вернул.
Как такое произошло пока не ясно.

@fresk-nc
Copy link
Contributor

Почитал внимательно комменты _updateHTML, кажется, что это запланированное поведение

@chestozo
Copy link
Member Author

У нас был как-то nullreference тут.
Предлагаю пока оставить.

@chestozo
Copy link
Member Author

chestozo commented Feb 6, 2017

Возможно, тоже связано #579

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

No branches or pull requests

3 participants