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

Attempt to fix https://github.com/reactjs/react-router/issues/3241 #267

Merged
merged 2 commits into from Apr 14, 2016

Conversation

2 participants
@slorber
Contributor

slorber commented Apr 5, 2016

No description provided.

@@ -39,7 +39,7 @@ function createBrowserHistory(options={}) {
key = history.createKey()
if (isSupported)
window.history.replaceState({ ...historyState, key }, null, path)
window.history.replaceState({ ...historyState, key }, null, window.location.href)

This comment has been minimized.

@taion

taion Apr 7, 2016

Contributor

The 3rd arg here should just be omitted, e.g.

window.history.replaceState({ ...historyState, key }, null)

This comment has been minimized.

@slorber

slorber Apr 7, 2016

Contributor

thanks yes I think it should work too, I'll try tomorow and edit my PR

@slorber

This comment has been minimized.

Contributor

slorber commented Apr 8, 2016

@taion I've tested and yes it seems to work fine for my usecase with your suggestion so I've updated the PR

@slorber

This comment has been minimized.

Contributor

slorber commented Apr 8, 2016

Just also want to point out that this code in useBasename.js:

    // Automatically use the value of <base href> in HTML
    // documents as basename if it's not explicitly given.
    if (basename == null && _ExecutionEnvironment.canUseDOM) {
      var base = document.getElementsByTagName('base')[0];

      if (base) basename = _PathUtils.extractPath(base.href);
    }

It does not work to automatically solve my usecase so I have to configure manually the basename. Anyway not sure there's anything clever to be done here. Maybe history could automatically detect that basename has a different domain than current url and issue a meaningful error to tell the user to configure a basename?

I tried if (base) basename = base.href; but it's not really what we want here because actually the history basename should be set to the backend server for solving the security issue, while my page has base=frontend server.

Something like that might work:

var base = document.getElementsByTagName('base')[0];
if (base ) {
  // For most history/RR users 
 // base can be a relative path, or an absolute path of the same domain as the current page
  if ( isLocationDomain(base) {
    basename = _PathUtils.extractPath(base.href);
  } 
  // For my usercase. Should not impact other users but make possible to solve my usecase without any configuration (with possibility to override if needed)
  else {
    basename = getLocationDomain();
  }
}

What do you think? I can make a new PR for that if you want.

@taion

This comment has been minimized.

Contributor

taion commented Apr 8, 2016

See the recent discussion on #94. The <base href> support in this library is just not correct and needs to be replaced entirely (e.g. with <base basename> or something).

@taion

This comment has been minimized.

Contributor

taion commented Apr 8, 2016

From #94 (comment) onward.

@slorber

This comment has been minimized.

Contributor

slorber commented Apr 14, 2016

@taion @mjackson @ryanflorence I've explained my issue more in-depth in ReactTraining/react-router#3241 (comment)

Now I don't want to rush you guys but I have to put something in production in 8 days :) so I need to know if you will consider merging this fix and release 2.0.2 soon, or if I should consider publishing my own fork at least temporarily

Thanks

@taion taion merged commit 7280ed9 into ReactTraining:v2.x Apr 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@taion

This comment has been minimized.

Contributor

taion commented Apr 15, 2016

@slorber Would you mind adding a regression test here? The triggers for the bug are obscure enough that is a nontrivial risk of a regression later on without such a test.

@slorber

This comment has been minimized.

Contributor

slorber commented Apr 15, 2016

ok i'll try to check how this can be tested :) if you have ideas plz tell me

@taion

This comment has been minimized.

Contributor

taion commented Apr 15, 2016

Give me a sec.

@taion

This comment has been minimized.

Contributor

taion commented Apr 15, 2016

I added it: #274.

@slorber

This comment has been minimized.

Contributor

slorber commented Apr 15, 2016

great thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment