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

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 8 commits into from

Conversation

ieure
Copy link

@ieure ieure commented Nov 22, 2016

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
Copy link
Member

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
Copy link
Author

ieure commented Dec 14, 2016

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
Copy link
Member

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
Copy link

solussd commented Feb 1, 2017

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
Copy link
Member

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
Copy link
Author

ieure commented Feb 27, 2017

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
Copy link
Author

ieure commented Feb 27, 2017

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
Copy link

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
Copy link

Is #25 a simpler fix?

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

Successfully merging this pull request may close these issues.

5 participants