brianleroux / lawnchair

A lightweight clientside JSON document store,
http://brianleroux.github.com/lawnchair
MIT License
2.13k stars 246 forks source link

Lawnchair 2 ideas from @felixhammerl #208

Closed brianleroux closed 6 years ago

brianleroux commented 9 years ago
Add linting

Yes pls!

Unify code style (strict mode, indent w/ four spaces, no comma-first, semicolons, curly braches in conditional statements, ...)

No. This is my project and it follows my coding style. I respect other projects with pull requests in their style. That's what the linter can do! If anything these days I go: no semicolons, 2 spaces, comma first, single line conditionals. This is a thought out style not blindly following old advice.

Remove new Function used in the terse callbacks

No. Its there for a reason. Before Crockford vilified eval it was very common pattern in JS. Don't beleive hype. It is useful to understand your tools: all of them.

Add continuous integration (e.g. via tastling) for multiple browser configurations

Travis CI is here. Testling is no longer maintained, alas.

Port the tests to mocha+chai if necessary

I'd rather we move to Tape instead. No globals. No augmented built ins.

Port lawnchair to promises (if and where that makes sense) to improve exception handling

No. This would require a dependency and the various libs that do this do it in diffferent ways. Then if in userland someone chooses a different lib you run into conflicts. We see it in PhoneGap a lot. Promises can come when they are native to the language. And quite bluntly, they add exactly zero value IMO but they do slow down code and create hard to follow stack traces.

Resume proper semantic versioning and publishing to npm, etc.

Agree!

Remove obsolete incomplete code paths across the modules

Agree!

felixhammerl commented 9 years ago

Add linting

Yes pls!

ACK

Unify code style (strict mode, indent w/ four spaces, no comma-first, semicolons, curly braches in conditional statements, ...)

No. This is my project and it follows my coding style. I respect other projects with pull requests in their style.

Quite frankly, i think the npmjs coding style is ugly, but ... just my opinion

That's what the linter can do! If anything these days I go: no semicolons, 2 spaces, comma first, single line conditionals. This is a thought out style not blindly following old advice.

The reason i find npmjs code style problematic is minification. omitted semicolons have been known to break concatenation and minification. Right now, the files have different code styles throughout, so ANY code style is better than none, so I don't really care as long as it's consistent. Ideally this process is automated via beautification.

Remove new Function used in the terse callbacks

No. Its there for a reason. Before Crockford vilified eval it was very common pattern in JS. Don't beleive hype. It is useful to understand your tools: all of them.

eval sucks for a lot of reasons: arbitrary code execution, runtime performance, debugging goes nuts, content-security-policy, and if you use minification, i guarantee that it will blow up.

but it's not a lot of code and if you want to keep it, fine by me.

Add continuous integration (e.g. via tastling) for multiple browser configurations

Travis CI is here. Testling is no longer maintained, alas.

I just wanna run the tests on the last two generations of the major browsers, preferrably on Travis. Saucelabs and browserstack cost, even for OS projects. Dalek seems overkill.

Other Ideas?

Port the tests to mocha+chai if necessary I'd rather we move to Tape instead. No globals. No augmented built ins.

In most of my project, I use mocha+chai+sinon because it handles async tests like a charm. No preference though.

Port lawnchair to promises (if and where that makes sense) to improve exception handling

No. This would require a dependency and the various libs that do this do it in diffferent ways. Then if in userland someone chooses a different lib you run into conflicts. We see it in PhoneGap a lot. Promises can come when they are native to the language.

Agree 100% that we should not ship with yet another lib. But there's this awesome framework called Vanilla JS that comes with ES6 Promises :) If need be, they can be polyfilled w/ Jake Archibald's polyfill.

And quite bluntly, they add exactly zero value IMO

They do add value. What I like about Promises is that you can properly throw and catch errors, as you would in any other language. Firefox throws when you fire up an IndexedDB in read-only mode, which would have been caught. On the other hand, IndexedDB/WebSQL just do not work the way Promises do, so that feels a bit like pushing the square peg through the round hole.

but they do slow down code

No, they don't. Or better: Not anymore.

and create hard to follow stack traces.

ACK, but that's for future generations of dev tools to solve.

No strong preference for or against Promises. Would be further down the road anyway, so might as well just not do it for now.

Resume proper semantic versioning and publishing to npm, etc.

Agree!

ACK

Remove obsolete incomplete code paths across the modules Agree!

ACK

brianleroux commented 9 years ago

Things we agree on and should just start doing:

Some additional comments. eval is fine and quite necessary for cold code loading which most non trivial apps do (xhr + eval). Arbitrary code exec is the actual feature so idk why this is a problem. I've heard tales of rogue ad networks but I have no business that requires advertising revenue so I haven't run intot hat myself. Perf hasn't been an issue since before v8 existed. Debugging is an excuse for ppl whom don't write tests, and I use minification all the time, for over a decade come to think of it, without it blowing up.

BUT content-security-policy is legit. =)

Promises. They DO slow down things when they are not native. >1kb of code is slower than 0kb. They create another object on the heap and that allocation adds up in async code creating work for the garbage collector potentially inviting skipped frames in the render paint. I wasn't kidding: userland polyfills, vanillajs included, are a serious clusterfuck for us in phonegap land. So much so, I'll revert any attempt to put them within reach of any code I ship until they are native to all browsers and runtimes I'm supporting. Good news: that will not be a very long time. Better news: you don't need them anyhow. They are syntactic sugar at best.

ANYHOW! Lets focus on the first stuff (if you agree) and I'll add you as a committer now. Feel free to ping me anytime. I'll try to jump in as I can but have a lot of these projects in my orbit chewing time!

felixhammerl commented 9 years ago

Things are going haywire here, so you guys go ahead w/o me for now. I can't promise when I'll have the resources and the time to work on this. F***. :(