search

kibu-australia / pushy

Clojurescript library for quick and easy HTML5 pushState
Eclipse Public License 1.0
223 stars 28 forks source link

Update pushy to allow SPA apps w/ URI fragments. #21

Open ieure opened 7 years ago

ieure commented 7 years ago

Here's a SWAG at how I think this should work. It's passed an extremely rigorous test suite, by which I mean it wasn't completely broken when I tried it on my laptop exactly once.

re #14

thomasmulvaney commented 7 years ago

The tests didn't even run at all on Travis although it confusing says it passed. I suggest you take a look at errors from the build.

You'll also need to add tests for the new functionality. So we can see how the new functionality works and so we don't regress in the future.

Thanks!

ieure commented 7 years ago

I'm happy to do this, but can you please take a look at the overall approach and give me feedback on that?

I've had occasions where I've gone to great lengths, including printing out a contributor agreement, signing it, getting my employer to sign it, and physically mailing it out, only to be told that there was no chance the changes or anything like them would ever be accepted. So I'd like to get opinions on the actual changes before investing effort that's ultimately wasted.

thomasmulvaney commented 7 years ago

Overall I like the approach. My only issue so far is that the new-history has lost its 0-arity version. I like that path-prefix is passed in via the options map.

Thanks for working on this. Hope you can make it work!

solussd commented 7 years ago

I'd like to see this merged, too. Is the only remaining issue the now missing 0-arity new-history? That seems like a trivial change. Happy to make it if it unblocks this PR.

thomasmulvaney commented 7 years ago

As mentioned earlier, the approach looks good but needs tests. Travis has green lighted, but the CLJS failed to build: https://travis-ci.org/kibu-australia/pushy/builds/177871223

ieure commented 7 years ago

Okay, so. I spent literally hours of my weekend trying to make a test that passed, but I'm out of time and patience. I fixed a couple issues & added tests for some things, but the async test of dispatch etc refuses to pass. I think it might not be possible, since secretary's config is global, but the core & hash-based tests run at the same time.

ieure commented 7 years ago

There's a non-passing test if someone wants to figure out how to wrangle this mess. Or I can remove it, leaving the tests for the new things I added, which are in the existing core tests.

dustingetz commented 7 years ago

This approach forces a choice between fragment navigates and html5 navigates. If the flag is not set, pushy should let the native event through unhandled (no dispatch). It should not drop the event.

dijonkitchen commented 6 years ago

Is #25 a simpler fix?