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

location: change url on location:setLocation #182

Closed
wants to merge 1 commit into from

Conversation

yoshuawuyts
Copy link
Member

I think this should address the issues from #85. Hash routes probably won't work well, but in order to resolve that we'd probs have to patch the router (yoshuawuyts/sheet-router#31).

I've tested this on the mailbox example and it seems to work well. Passing in two send() calls does mean there's a slight delay in the UI, but I think we can get around that by removing setTimeout() in barracks - which would probably not unleash Zalgo. Maybe it would unleash Zalgo. I think we should leave that up to the consumers to take care of hah.

Anyway, I hope this resolves the outstanding routing issues. Feedback def welcome, as it'd be cool to get this fix right(ish). Cheers!

@timwis
Copy link
Member

timwis commented Jul 20, 2016

But hashes! I feel like people think of hash routing as something just for old browsers, but what about sites on github pages and other static servers that don't support "overriding the resource and passing it to index.html"? I would advocate for hash routing being treated as a first class citizen - unless there's some other way to use it widespread?

Couldn't we just do someting like if (opts.hash) window.location.hash = xxx; else window.pushState()..; send(...)?

@yoshuawuyts
Copy link
Member Author

yoshuawuyts commented Jul 20, 2016

But hashes! I feel like people think of hash routing as something just for old browsers

No no, not at all. I know it's like the only proper way to do routing in electron for example. But we can't get everything in one go, and this at least fixes the issues for people using path. Hashes are not forgotten, but need to wait I think

edit:

Couldn't we just do someting like if (opts.hash) window.location.hash = xxx; else window.pushState()..; send(...)?

ehhh, yeah maybe. I need food. Going to think about this for a bit

@yoshuawuyts
Copy link
Member Author

You're totally right - this patch would break hash routing - gotta fix it! :D

@paul-veevers
Copy link

paul-veevers commented Jul 21, 2016

Hey guys, been lurking for a while and loving Choo. Currently building a proof-of-concept third-party embeddable widget (i.e. sits on other people's websites).

One requirement is that it doesn't change the URL of the website (hash or otherwise). But obviously I still need routing. This has worked perfectly so far:

const tree = app.start({ history: false, href: false, hash: false, });
send('location:setLocation', {location: '/'}, done);

This PR would break this functionality? I.e. it would change the URL.

I can see I will be able to call the updateHref reducer directly instead? But thought I'd point out a use case that will break 👍

Look forward to contributing a bit more when I have some breathing room :)

@yoshuawuyts
Copy link
Member Author

@paul-veevers I hear you - yeah we should take care of that for sure

@yoshuawuyts
Copy link
Member Author

yoshuawuyts commented Jul 26, 2016

Update: not forgotten about this, life is just a teeny tiny mess at the moment. More than happy to accept PRs against this PR to solve this - might be a few more days still, otherwise. Thanks for your patience ✨

@yoshuawuyts
Copy link
Member Author

yoshuawuyts commented Jul 30, 2016

Been tinkering with this, and there's almost no way to escape breaking changes - going to close this issue and put out a full router revamp patch as we might as well go all out then ✨ - sorry again for the wait!

@yoshuawuyts yoshuawuyts deleted the set_url_on_change branch August 15, 2016 13:49
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.

3 participants