Skip to content
This repository was archived by the owner on Dec 15, 2018. It is now read-only.

Conversation

@tptee
Copy link
Contributor

@tptee tptee commented Jul 13, 2016

This should end our API churn.

  • <Link> now respects modifier keys!
  • <Link> now accepts either a string or an object like so:
{
  pathname: '/yo/this/is/a/url',
  query: {
    some: 'stuff'
  }
}
  • <Link> comes in two flavors now: <Link> (old behavior) and <PersistentQueryLink> (persists the previous query string unless the href prop provides one). Since you'll probably just use one or the other, this is a nice pattern:
import { PersistentQueryLink as Link } from 'redux-little-router';
  • Query string support is fixed across the board!
  • Added a util that sets up initial state on a server-side render. This correctly sets up query strings on the initial location dispatch.
  • The history listener now lives in the store enhancer. The middleware no longer lives in the enhancer and must be used separately due to middleware order issues.
  • The store enhancer's reducer enhancer (enhance!!!) now supports redux-loop.
  • The current route info now lives at the top of the router state tree like it always should've (no more router.current). You can still access the previous route on router.previous.
  • New Fragment component that shows or hides children based on active routes (and even conditions of those active routes).
  • New provideRouter adds the router context to a top-level component. This means that <Link> no longer requires you to pass in dispatch and history (THANK GOD).

src/link.js Outdated
};

const clickedWithModifier = e =>
e.button === LEFT_MOUSE_BUTTON &&

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you want to check for LEFT_MOUSE_BUTTON here. (I haven't tested this, so could be wrong).

This behavior should happen if you're clicking a link by pressing enter on the keyboard as well.

We'll also want to check for right clicks and not trigger any behavior on them. This module includes some logic that handles this. Don't know if we want to use the whole thing or not because it does some other stuff too: https://github.com/lukekarrys/local-links/blob/master/local-links.js#L26-L28

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally right, not sure what I was thinking there 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Latest commit should do the trick! Tested with the enter key: uses pushState without modifiers, default if using modifiers 👍 Also checking for the target prop

@coveralls
Copy link

coveralls commented Jul 13, 2016

Coverage Status

Coverage decreased (-4.7%) to 91.339% when pulling d7bc687 on feature/better-link into 2fbf246 on master.

@coveralls
Copy link

coveralls commented Jul 13, 2016

Coverage Status

Coverage decreased (-4.7%) to 91.339% when pulling d7bc687 on feature/better-link into 2fbf246 on master.

@coveralls
Copy link

coveralls commented Jul 13, 2016

Coverage Status

Coverage decreased (-4.7%) to 91.339% when pulling 6189992 on feature/better-link into 2fbf246 on master.

tptee added 2 commits July 13, 2016 16:46
- Use the history query enhancer for query strings
- router.current now lives on router
- much better link
- store enhancer no longer owns middleware
@tptee tptee force-pushed the feature/better-link branch from 6189992 to 970e68d Compare July 13, 2016 23:50
@coveralls
Copy link

coveralls commented Jul 13, 2016

Coverage Status

Coverage decreased (-4.7%) to 91.339% when pulling 970e68d on feature/better-link into 2fbf246 on master.

import createMatcher from './create-matcher';

export default ({ url, query, routes, history }) =>
Object.assign({},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No versions of IE support Object.assign, and it doesn't appear to be included in es5-shim either. So we'll either need to call it out that you need an assign polyfill, don't support IE (not really an option for us), or pull in _.assign. I vote for the last option.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ew! Definitely the lodash one.

@coveralls
Copy link

coveralls commented Jul 14, 2016

Coverage Status

Coverage decreased (-6.2%) to 89.844% when pulling ecf2877 on feature/better-link into 2fbf246 on master.

@divmain
Copy link
Contributor

divmain commented Jul 14, 2016

A couple of questions, but the rest LGTM! No need for re-review once those are addressed.

@coveralls
Copy link

coveralls commented Jul 14, 2016

Coverage Status

Coverage decreased (-6.2%) to 89.844% when pulling ecf2877 on feature/better-link into 2fbf246 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-6.1%) to 89.922% when pulling 02c1826 on feature/better-link into 2fbf246 on master.

@coveralls
Copy link

coveralls commented Jul 14, 2016

Coverage Status

Coverage decreased (-6.1%) to 89.922% when pulling 02c1826 on feature/better-link into 2fbf246 on master.

.that.contains('/home/messages/a-team');
expect(payload).to.have.property('query')
.that.deep.equals({ test: 'ing' });
done();
Copy link
Contributor

@divmain divmain Jul 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the expect statements above fail, the test will timeout. This is because an Error is thrown, but it is thrown outside the stack frame of the it block (its asynchronous). There are a couple of ways you can resolve this:

  1. Wrap all async expect statements in a try...catch block, and pass any caught errors to done. In other projects, I've done this with a helper function that looks like captureErrors(done, () => { /* expect statements here */ });. This function calls done with a thrown error if it occurs, and otherwise calls done with no args.
  2. Return Promise.resolve().then(() => { /* expect statements here */ }); in the it callback. If the promise rejects, the test will fail with the rejection reason.
  3. Make the test only fake-synchronous. This isn't always possible or desirable, because often library code is asynchronous and outside of your control.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const isNotLeftClick = e => e.button !== LEFT_MOUSE_BUTTON;

e.button is undefined in enzyme simulated clicks. This means that we never reach the real <Link> behavior. Here's the fix:

const isNotLeftClick = e => e.button && e.button !== LEFT_MOUSE_BUTTON;

😆

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(the expects should still be wrapped, but that can be done in a separate PR)

@tptee tptee merged commit b888883 into master Jul 14, 2016
@tptee tptee deleted the feature/better-link branch July 14, 2016 17:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants