Allow <Link> to be used outside of Router, add warning #3896

Connorelsea · 2016-09-17T22:04:12Z

Fixes #3889

Previously, using a Link outside of the Router would break the program. The breaking was caused by the inability to find location when outside of the router context. This PR fixes that by checking if the context contains the router, and if not, marking "isActive" as false by default. This introduces the assumption that all outside of the router are not active.

This also adds a warning to indicate that the user is using the is being used outside of the error. I am not exactly sure what copy to use in the error.

As of right now, since there is no invariant in handleClick, a can be "used" outside of a router. It simply reloads the page based on the href.

This is my first time contributing, so let me know if there is anything else that needs to be changed or added to resolve this issue.

Connorelsea · 2016-09-17T22:49:33Z

I have a few questions:

What utility should I use to display an warning, since console.warn makes the Travis build fail. And what should the error say exactly?
Should I add an invariant in handleClick to prevent links from being clicked if they are outside of the Router? If so, it shows the error and then stills goes to the new page because of the href value.

ryanflorence · 2016-09-18T00:40:18Z

let's just not handle clicks and not check isActive without a router in
context.
On Sat, Sep 17, 2016 at 3:49 PM Connor Elsea [email protected]
wrote:

Should I add an invariant in handleClick to prevent links from being
clicked if they are outside of the Router? If so, it shows the error and
then stills goes to the new page because of the href value.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#3896 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAGHaCyTAo-SdGv8yzjZWfxiGz7WHFntks5qrG5-gaJpZM4J_uhR
.

ryanflorence · 2016-09-18T00:40:36Z

(was intended but just an oversight)
On Sat, Sep 17, 2016 at 5:40 PM Ryan Florence [email protected] wrote:

let's just not handle clicks and not check isActive without a router in
context.
On Sat, Sep 17, 2016 at 3:49 PM Connor Elsea [email protected]
wrote:

Should I add an invariant in handleClick to prevent links from being
clicked if they are outside of the Router? If so, it shows the error and
then stills goes to the new page because of the href value.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#3896 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAGHaCyTAo-SdGv8yzjZWfxiGz7WHFntks5qrG5-gaJpZM4J_uhR
.

Connorelsea · 2016-09-18T02:26:03Z

@ryanflorence Do we want to warn users that it is not handling clicks? If so, where, onClick? Because when you click on a Link outside of a Router it still clicks and goes to the href.

Connorelsea · 2016-09-18T03:21:17Z

Rebased to remove the warning. Now this commit refactors the method signature of getIsActive to allow for the sniffing of router context. Then getIsActive returns false if the router is not available on the context. It does this without any warning to the user. This also refactors away verbosity from the className assignment when returning the final anchor tag.

As of right now clicking on this link outside of the router just follows its href. It seems like there is no problem occurring with handling onClick.

timdorr · 2016-09-18T03:57:07Z

modules/Link.js

@@ -1,4 +1,5 @@
 import React, { PropTypes } from 'react'
+import invariant from 'invariant'


I'd actually use this if you're importing it ;)

timdorr · 2016-09-18T03:59:18Z

modules/Link.js

-        location.query
-      )
+    isActive: (location, to, props, router) => {
+      if (router) {


I wouldnt check for router here. Move it up to wrap the construction of all props for the eventual a tag. I.e., in the case of no router context, this renders a normal a tag.

@timdorr Do you just want to spread the given props of the Link onto an anchor when there is no router and return that? Also, should it warn that it is doing this? If so, how should we go about firing a warning, console.warn causes the build to fail

@timdorr If we just spread, to won't ever get transformed to href or anything.

Connorelsea · 2016-09-18T06:24:16Z

Hey @timdorr and @ryanflorence, I have iterated on this and feel as if I have found a solution that I feel will benefit users.

Fixes in Link

When outside of a <Router> parent context, <Link>s now do the following:

In handleClick(): PerformonClick if it is explicitly specified as a prop to a <Link>
In handleClick(): Warn after onClick action that there is no <Router> parent
In handleClick(): Prevent router.transitionTo from happening (due to no router context)
In render(): If no router, set isActive to false

Effect of these changes

A <Link> that somehow ends up outside of a router will still function as normal. It will load pages based on the href or a supplied onClick method. Yet it will not perform the transitions or check the links "activeness" as it attempted to do previously, which caused errors outside of the router context. Since some of the expected features are missing, and placing a outside of the Router may cause un-intended side-effects, the user should be warned that these feature omissions are occurring.

Questions

How should I go about issuing this warning to users. console.warn causes a build fail. See here for my comment in the code on using a warning.

ryanflorence · 2016-09-19T03:32:55Z

modules/Link.js

+        event.preventDefault()
+        this.context.router.transitionTo(this.props.to)
+      }
+    } else {


just drop the else, most likely we're just in a unit test and we don't want warnings anyway.

ryanflorence · 2016-09-19T03:34:26Z

modules/Link.js

@@ -79,12 +87,15 @@ class Link extends React.Component {
    } = this.props

    const currentLocation = location || this.context.location
+    let isActive = false


const isActive = router ? getIsActive(...) : false

I think avoiding a let is worth a ternary, but idc that match, do it however you like.

ryanflorence · 2016-09-19T03:36:08Z

modules/Link.js

-        className={isActive ?
-          [ className, activeClassName ].join(' ').trim() : className
-        }
+        className={className + (isActive ? " " + activeClassName : "")}


This will create classNames like " active" and people will get mad, so trim it or just put it back.

for instance, when there is no className -> <Link activeClassName="active"/>

Connorelsea · 2016-09-19T03:55:17Z

@ryanflorence I made the suggested changes, but will have to look into why the tests failed. It is working perfectly fine for me locally when I build my changes and test RR using my build.

Connorelsea · 2016-09-19T03:58:05Z

Was it testing that the activeStyle is applied when not in the Router context? If so, these tests now fail because active is assumed to be false when there is no Router context.

timdorr · 2016-10-02T17:08:17Z

@Connorelsea I'm going to close this out, as I think we're close to good with the current version. Once #3986 lands, it basically does the same thing. But it's updated against recent changes. If it doesn't work, we'll need to rebase and refactor this branch anyways.

Connorelsea force-pushed the warn-on-link-outside-router branch from 30cca4b to 377fc86 Compare September 18, 2016 03:17

timdorr requested changes Sep 18, 2016

View reviewed changes

ryanflorence requested changes Sep 19, 2016

View reviewed changes

Connorelsea force-pushed the warn-on-link-outside-router branch 2 times, most recently from 5026c5c to d970305 Compare September 19, 2016 03:47

Allow <Link> to be used outside of Router, add warning

cc14309

Connorelsea force-pushed the warn-on-link-outside-router branch from d970305 to cc14309 Compare September 19, 2016 03:49

timdorr modified the milestones: v3.0.0, v4.0.0 Sep 28, 2016

timdorr mentioned this pull request Oct 2, 2016

Links apathetic to active don't subscribe (#3985) #3986

Merged

timdorr closed this Oct 2, 2016

lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow <Link> to be used outside of Router, add warning #3896

Allow <Link> to be used outside of Router, add warning #3896

Connorelsea commented Sep 17, 2016 •

edited

Loading

Connorelsea commented Sep 17, 2016 •

edited

Loading

ryanflorence commented Sep 18, 2016

ryanflorence commented Sep 18, 2016

Connorelsea commented Sep 18, 2016

Connorelsea commented Sep 18, 2016

timdorr Sep 18, 2016

timdorr Sep 18, 2016

Connorelsea Sep 18, 2016

Connorelsea Sep 18, 2016

Connorelsea commented Sep 18, 2016 •

edited

Loading

ryanflorence Sep 19, 2016

ryanflorence Sep 19, 2016

ryanflorence Sep 19, 2016

ryanflorence Sep 19, 2016

Connorelsea commented Sep 19, 2016

Connorelsea commented Sep 19, 2016

timdorr commented Oct 2, 2016

		@@ -1,4 +1,5 @@
		import React, { PropTypes } from 'react'
		import invariant from 'invariant'

Allow <Link> to be used outside of Router, add warning #3896

Allow <Link> to be used outside of Router, add warning #3896

Conversation

Connorelsea commented Sep 17, 2016 • edited Loading

Connorelsea commented Sep 17, 2016 • edited Loading

ryanflorence commented Sep 18, 2016

ryanflorence commented Sep 18, 2016

Connorelsea commented Sep 18, 2016

Connorelsea commented Sep 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Connorelsea commented Sep 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Connorelsea commented Sep 19, 2016

Connorelsea commented Sep 19, 2016

timdorr commented Oct 2, 2016

Connorelsea commented Sep 17, 2016 •

edited

Loading

Connorelsea commented Sep 17, 2016 •

edited

Loading

Connorelsea commented Sep 18, 2016 •

edited

Loading