Raynos / mercury

A truly modular frontend framework
http://raynos.github.io/mercury/
MIT License
2.82k stars 143 forks source link

Update virtual-dom #197

Closed gcallaghan closed 8 years ago

gcallaghan commented 8 years ago

I ran the tests with the new dependency and verified that my application ran as expected. I'm not sure if there is something else that is needed.

yoshuawuyts commented 8 years ago

if tests are passing, and you've tested it in a real world application I think this should be :+1:

crabmusket commented 8 years ago

Wow, I thought there was supposed to be a lot more fuss.

gcallaghan commented 8 years ago

Can this get merged? Is there more I should do?

ashnur commented 8 years ago

I think it would be great to confirm if all the examples work.

gcallaghan commented 8 years ago

Great point! It crossed my mind before the 9-5 demanded my attention. I'll get on it.

gcallaghan commented 8 years ago

Updated commit to include more dependencies and verified that all the examples work when https://github.com/maxogden/javascript-editor/issues/11 is resolved.

Raynos commented 8 years ago

:-1: We do not update all dependencies in one commit.

If certain dependencies have to be updated, we update them ideally one by one or groups at a time.

gcallaghan commented 8 years ago

fair enough. I'll reduce the scope of the commit.

Raynos commented 8 years ago
npm http fetch GET https://github.com/tmcw/CodeMirror/archive/module-export.tar.gz
npm http fetch 404 https://github.com/tmcw/CodeMirror/archive/module-export.tar.gz
npm ERR! fetch failed https://github.com/tmcw/CodeMirror/archive/module-export.tar.gz

This sucks :(

gcallaghan commented 8 years ago

@raynos, that's why I opened the related pull request first

debugging that is the only real reason I attempted to open the scope of the change.

gcallaghan commented 8 years ago

Reduced the scope of the commit to only include virtual-dom

Raynos commented 8 years ago

Last time i tried to upgrade virtual-dom it broke the examples.

Someone has to check the examples work locally and really play around with them.

gcallaghan commented 8 years ago

I ran the examples with a locally linked javascript-editor package. They all seem to work. No errors in the console and content updates as I would expect. If something is broken it's definitely not obvious. Anything to look for in particular?

Raynos commented 8 years ago

@gcallaghan the only thing to look for the is the TodoMVC example; play around with it for editing, adding, removing.

gcallaghan commented 8 years ago

TODO MVC example works except for the clear completed, however, this is broken on master as well.

Raynos commented 8 years ago

Clear completed in this build ( http://raynos.github.io/mercury/examples/todomvc/index.html ) seems to work ok.

gcallaghan commented 8 years ago

I'll take a look to see why my local build is different

gcallaghan commented 8 years ago

There's a difference on the build you pointed me to @Raynos

The hosted build has the following function which works.

function clearCompleted(state) {
   state.todos().forEach(function (todo, index) {
        if (todo.completed) {
            destroy(state, todo)
        }
    })
}

Master contains the following code, that I receive when running npm run examples

function clearCompleted(state) {
    Object.keys(state.todos).forEach(function clear(key) {
        if (TodoItem.isCompleted(state.todos[key])) {
            destroy(state, state.todos[key]());
        }
    });
}

This breaks because 'set' is returned as a key of state.todos.

I think this is unrelated to this pull request.

Raynos commented 8 years ago

Let's just get this in; fix forward anything else we find :)

Raynos commented 8 years ago

Published v14.1.0