search

martyjs / marty

A Javascript library for state management in React applications
http://martyjs.org
MIT License
1.09k stars 76 forks source link

Marty v0.9 #137

Closed jhollingworth closed 9 years ago

jhollingworth commented 9 years ago

Improvements

Bugs

Tasks

dariocravero commented 9 years ago

This looks like a very good step forward :). Need any help? Also, should we make marty ES6-proof using something like Babel (previously 6to5) if no native ES6 support is available?

jhollingworth commented 9 years ago

Hey, yeah would love some help :smile: Would you be interested in picking up #132? I was playing around with it the other day but got distracted...

Completely agree on Babel, I started re-writing everything to be ES6 classes the other day. It makes isomorphisim a whole load easier if everything's instantiatable. Things like Marty.createStore will still be supported but I'm adding Marty.register(SomeStore) for ES6 classes. Hopefully will be done by next week...

FoxxMD commented 9 years ago

I'd be happy to help test when you need it :+1:

dariocravero commented 9 years ago

Good stuff! :) Will try to tackle #132 over the next few days. As for Babel, #138 might be a good a start. :) <3 to see an es6 branch already! :)

Dakuan commented 9 years ago

This looks awesome - we are really hoping to use marty for new project but lack of server rending is a dealbreaker. Can pitch in some support testing things.

dariocravero commented 9 years ago

@jhollingworth quick one, have you considered migrating the testing suite to jest?

jhollingworth commented 9 years ago

Jest is cool however it would require converting all tests from mocha to jasmine. Also, I don't think it integrates well with SauceLabs (thats based on a quick google so could be wrong).

dariocravero commented 9 years ago

@jhollingworth on mocha/jest: facebook/jest#139

Dakuan commented 9 years ago

Just had a quick look at this to have a play with the server rendering but having some issues with webpack:

First I got this sort of thing:

ERROR in ./~/marty/lib/actionCreators/actionCreators.js
Module parse failed: /Users/Dom/git/anu/node_modules/marty/lib/actionCreators/actionCreators.js Line 15: Unexpected reserved word
You may need an appropriate loader to handle this file type.
| }
|
| class ActionCreators {

which I'm guessing is the es6 stuff. I added babel-loader to my webpack config:

{
    test: /\.(es6|jsx|js)$/,
    loader: 'babel-loader'
}

which makes the errors go away - but breaks the webpack-dev-server. I see this in the browser console:

 Uncaught TypeError: Cannot use 'in' operator to search for 'document' in undefined
jhollingworth commented 9 years ago

@Dakuan does it tell you what line that error is thrown on?

dariocravero commented 9 years ago

@Dakuan can you paste your full webpack.config.js?

Dakuan commented 9 years ago

@jhollingworth @dariocravero

managed to fix with this webpack config:

{
            test: /\.(jsx|es6|js)$/,
            loader: 'babel-loader',
            include: /(?=src)|(?=node_modules\/marty)/
        }

and by disabling the marty chrome plugin

jhollingworth commented 9 years ago

@Dakuan awesome! I going to start on a webpack demo soon so this will be useful :smile:

jhollingworth commented 9 years ago

Good point about Marty Developer Tools, have added it to the list of things to look at. Thanks

Dakuan commented 9 years ago

Yep! Some progress but not out of the woods yet. When trying to require (on the server) a file that is connected with the marty app eg some kind of entry point like app.jsx or routes.jsx you get an error as node wont like the class keyword being out of place.

A natural thing to do is to try and use the babel require polyfill from here https://babeljs.io/docs/usage/require/ . This still won't work yet as marty is ignored by babel due to it being in node_modules. That's easily fixed by adding the {ignore: true} flag to the babel config. It's dog slow first time - hopefully whitelisting marty rather than just allowing all the npm modules to be parsed will help, but it's not working as documented at the moment.

Anyway, that spins for a while and then we get:

-> node src/server/index.js

/Users/Dom/git/anu/node_modules/marty/node_modules/underscore/underscore.js:1536
}.call(this));
  ^
TypeError: Cannot read property '_' of undefined
    at /Users/Dom/git/anu/node_modules/marty/node_modules/underscore/underscore.js:15:32
    at Object.<anonymous> (/Users/Dom/git/anu/node_modules/marty/node_modules/underscore/underscore.js:1536:3)
    at Module._compile (module.js:456:26)
    at normalLoader (/Users/Dom/git/anu/node_modules/babel/lib/babel/api/register/node.js:102:5)
    at Object.require.extensions.(anonymous function) [as .js] (/Users/Dom/git/anu/node_modules/babel/lib/babel/api/register/node.js:115:7)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Module.require (module.js:364:17)
    at require (module.js:380:17)
    at Object.<anonymous> (/Users/Dom/git/anu/node_modules/marty/index.js:4:9)

Here is how I get babel running:

// index.js 

require("babel/register")({
    ignore: false
});

var app = require('./app');

// 4130 is a steel alloy. It's what you make BMX frames out of.
app.listen(process.env.PORT || 4130, function() {
    console.log('listening on *:4130');
});
Dakuan commented 9 years ago

I should add that babel appears to be otherwise working, this test es6 class works as expected:

class Thing {
    log () {
        console.log('mew');
    }
}

module.exports = Thing;
dariocravero commented 9 years ago

I was about to suggest babel should work as I was reading the thread on my email @Dakuan and then saw your last comment :). You can always pipe your code thorugh babel-node; iojs might also be a better target than node going forward.

Dakuan commented 9 years ago

Which package do you mean? Can't seem to find it here: https://www.npmjs.com/search?q=babel-node

dariocravero commented 9 years ago

It's a binary that babel installs

jhollingworth commented 9 years ago

It might be easier if node consumes the built files rather than having to get people to add shims or use different JS environments. Although that has the downside of making it slightly more painful to debug

Dakuan commented 9 years ago

Yeah I tried requiring dist/marty but I got a babel error:

require("babel/register")({
    ignore: false
});

var app = require('./app');

gives:

/Users/Dom/git/anu/node_modules/marty/dist/marty.js:3241
global._babelPolyfill = true;
^
Error: only one instance of babel/polyfill is allowed
    at Object.<anonymous> (/Users/Dom/git/anu/node_modules/marty/dist/marty.js:3241:1)
    at Object.core-js/shim (/Users/Dom/git/anu/node_modules/marty/dist/marty.js:3246:66)
    at s (/Users/Dom/git/anu/node_modules/marty/dist/marty.js:1:637)
    at /Users/Dom/git/anu/node_modules/marty/dist/marty.js:1:679
    at Object.../../polyfill (/Users/Dom/git/anu/node_modules/marty/dist/marty.js:3234:30)
    at s (/Users/Dom/git/anu/node_modules/marty/dist/marty.js:1:637)
    at /Users/Dom/git/anu/node_modules/marty/dist/marty.js:1:679
    at Object../lib/babel/api/register/node (/Users/Dom/git/anu/node_modules/marty/dist/marty.js:5690:4)
    at s (/Users/Dom/git/anu/node_modules/marty/dist/marty.js:1:637)
    at /Users/Dom/git/anu/node_modules/marty/dist/marty.js:1:679

and without babel:

// require("babel/register")({
//     ignore: false
// });

var app = require('./app');

gives:

 -> node src/server/index.js

/Users/Dom/git/anu/node_modules/marty/dist/marty.js:7344
  if (self.fetch) {
      ^
ReferenceError: self is not defined
    at /Users/Dom/git/anu/node_modules/marty/dist/marty.js:7344:7
    at Object.<anonymous> (/Users/Dom/git/anu/node_modules/marty/dist/marty.js:7664:3)
    at s (/Users/Dom/git/anu/node_modules/marty/dist/marty.js:1:639)
    at /Users/Dom/git/anu/node_modules/marty/dist/marty.js:1:690
    at Object.whatwg-fetch (/Users/Dom/git/anu/node_modules/marty/dist/marty.js:7667:1)
    at s (/Users/Dom/git/anu/node_modules/marty/dist/marty.js:1:639)
    at /Users/Dom/git/anu/node_modules/marty/dist/marty.js:1:690
    at Object.../../http/hooks/parseJSON (/Users/Dom/git/anu/node_modules/marty/dist/marty.js:1762:1)
    at s (/Users/Dom/git/anu/node_modules/marty/dist/marty.js:1:639)
    at /Users/Dom/git/anu/node_modules/marty/dist/marty.js:1:690
Dakuan commented 9 years ago

I'm wondering if es6 is more trouble than its worth for a lib that wants to be able to run anywhere. :crying_cat_face:

jhollingworth commented 9 years ago

I've thought the same a few times recently... Once Marty.Component is done I'm going to focus on simplifying getting it to work on node. Hopefully this will be much better next week!

Thanks for all you're help trying this out, very much appreciated!

oliverwoodings commented 9 years ago

@jhollingworth could we add another dist output that is precompiled into ES5? By default marty could be es5, but if you want to use ES6 you could do require("marty/es6") or something

jhollingworth commented 9 years ago

We're feature complete on this release so I've publish Marty v0.9.0-rc1. I've updated the chat example (including isomorphism) if anyone is interested in what it looks like

dariocravero commented 9 years ago

Brilliant!! :) It's going to be an amazing release!

I was wondering if it should actually be 1.0.0 as it does include a fairly good amount of changes (despite the old mode just being deprecated). What would your plans for a 1.0.0 release be otherwise?

I'm working on a fully class-ified structure right now, will put that up as an example too. Will have a look at the chat example later on today and comment on it.

jhollingworth commented 9 years ago

Good point about v1.0. My only concern is React isn't stable yet so we might have to change the APIs (e.g. How we use contexts). Maybe it's worth waiting until React 1.0 comes out?

On 9 Mar 2015, at 1:19 pm, Darío Javier Cravero notifications@github.com wrote:

Brilliant!! :) It's going to be an amazing release!

I was wondering if it should actually be 1.0.0 as it does include a fairly good amount of changes (despite the old mode just being deprecated). What would your plans for a 1.0.0 release be otherwise?

I'm working on a fully class-ified structure right now, will put that up as an example too. Will have a look at the chat example later on today and comment on it.

— Reply to this email directly or view it on GitHub.

dariocravero commented 9 years ago

Good stuff :) In all fairness, 0.9.0 add functionality and is still backwards compatible, so it's alright :)

jhollingworth commented 9 years ago

Last major blocker (beyond pending tasks) is react-router v0.13 rackt/react-router#638

dariocravero commented 9 years ago

On that: #186 :). The way I see it, an agnostic framework wouldn't make assumptions as to what routing you want to use. Therefore react-router shouldn't be enforced and should be hidden behind a facade that allows any router to be plugged in. Am I wrong to think that marty might be better off being agnostic to have greater acceptance/usage and granularity? Thoughts?

jhollingworth commented 9 years ago

Oh completely. its just that marty-express currently only works with react-router (although plan is to allow any router to integrate). Isomorphisim is one of the major features for v0.9 so want everything working together before I release

dariocravero commented 9 years ago

Class! Good to know it was already on your mind too :) I'm glad to help towards getting the next version on a modular approach if that's desirable

FoxxMD commented 9 years ago

Along with #153 and #156 I'd like to add that there should be an UPGRADING.md document (or include in one of the docs) in the case that there are any breaking changes between 0.8 and 0.9.

dariocravero commented 9 years ago

I'll be running an upgrade over the next few weeks (probably next week) on a fairly large 0.8 project to 0.9 with classes. I think I should be able to create a guide out of that documenting the process and any gotchas in between. It shouldn't really break anything with your current code though. On 11 Mar 2015 15:06, "Matt" notifications@github.com wrote:

Along with #153 https://github.com/jhollingworth/marty/pull/153 and #156 https://github.com/jhollingworth/marty/issues/156 I'd like to add that there should be an UPGRADING.md document (or include in one of the docs) in the case that there are any breaking changes between 0.8 and 0.9.

— Reply to this email directly or view it on GitHub https://github.com/jhollingworth/marty/pull/137#issuecomment-78281271.

FoxxMD commented 9 years ago

Thanks @dariocravero that'd be awesome :+1:

jhollingworth commented 9 years ago

I've just finished a big update to Marty DevTools. Load of bug fixes, immutable.js support, a better view on the data plus the ability to revert actions

jhollingworth commented 9 years ago

v0.9 docs are available here http://martyjs.org/v/0.9.0-rc2/

dariocravero commented 9 years ago

@FoxxMD here's a rough guide (by example). Will try to write it up a bit better these days.

FoxxMD commented 9 years ago

thanks!