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 Aug 8, 2016

Infrastructure

  • Simplify config
  • Replace Karma with Mocha + nyc + babel-plugin-istanbul
  • Update travis.yml with new coverage command
  • Rewrite the test suite to actually use stubs/spies
  • Added coverage for <Fragment>

Patch

  • Fix a bug where <Fragment> ignored route precedence and would mistake concrete routes for parameters (e.g.<Fragment forRoute='/home' /> would display for routes like /:wantsparams)
  • Remove unused code for generating a full URL

Breaking changes

  • Remove the middleware and wrap dispatch in the store enhancer instead
  • Rename initialStateForSSR to initialRouterState and make it private
  • The store enhancer now handles initial location state when passed a pathname and/or a query object.
  • Provide a default history instance and remove need to provide a custom instance to both the store enhancer and the provider. Accept a basename option in the enhancer for useBasename. Accept a forServerRender flag to determine which history instance to use (memory or browser).

tptee and others added 5 commits August 7, 2016 17:58
- Replace Karma with Mocha + nyc + babel-plugin-istanbul
- Update travis.yml with new coverage command
- Remove the middleware and wrap dispatch in the store enhancer instead
- Remove unused code for generating a full URL
- Rewrite the test suite to actually use stubs/spies
@tptee tptee changed the title Feature/next 7.0.0 Aug 8, 2016
@tptee tptee changed the title 7.0.0 [WIP] 7.0.0 Aug 8, 2016
@coveralls
Copy link

coveralls commented Aug 8, 2016

Coverage Status

Coverage increased (+7.09%) to 98.214% when pulling a4f8f96 on feature/next into 326e923 on master.

@coveralls
Copy link

coveralls commented Aug 8, 2016

Coverage Status

Coverage increased (+7.09%) to 98.214% when pulling c92b7f2 on feature/next into 326e923 on master.

@ryan-roemer
Copy link
Member

Any plans for functional tests for the supported browser matrix now that we're completely simulating unit tests w jsdom?

Also, what's the word on the street of jsdom vs cheerio? Last I heard everyone was moving to cheerio. (Though haven't checked up in a while...)

@coveralls
Copy link

coveralls commented Aug 8, 2016

Coverage Status

Coverage increased (+7.1%) to 98.276% when pulling 24696b4 on feature/next into 326e923 on master.

@tptee
Copy link
Contributor Author

tptee commented Aug 8, 2016

Yep, functional tests will be on the agenda! I'm also working on an example project that we can reuse for those tests.

I'm still fuzzy on the differences between jsdom and cheerio. AFAICT, jsdom provides a headless environment + HTML parser, while cheerio provides an HTML parser plus a jQuery-equivalent API. Enzyme uses cheerio under the hood, so maybe we're OK?

@coveralls
Copy link

coveralls commented Aug 8, 2016

Coverage Status

Coverage increased (+7.1%) to 98.276% when pulling 12723fc on feature/next into 326e923 on master.

@coveralls
Copy link

coveralls commented Aug 8, 2016

Coverage Status

Coverage increased (+7.1%) to 98.276% when pulling 97f0d1f on feature/next into 326e923 on master.

@tptee tptee changed the title [WIP] 7.0.0 7.0.0 Aug 8, 2016
@tptee
Copy link
Contributor Author

tptee commented Aug 8, 2016

Alrighty, this is ready for review!

There's a couple of sinon smells in the test suite which I'll address in a later PR.

@tptee
Copy link
Contributor Author

tptee commented Aug 8, 2016

/cc @divmain @baer etc.

"rules": {
"no-unused-expressions": "off"
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason this is JSON, whereas the base .eslintrc is YAML?

@divmain
Copy link
Contributor

divmain commented Aug 8, 2016

LGTM!

@baer
Copy link
Contributor

baer commented Aug 8, 2016

this PR LGTM but it could use some docs.

@tptee
Copy link
Contributor Author

tptee commented Aug 8, 2016

Ask and ye shall receive! #22

@tptee tptee merged commit 72b08ca into master Aug 8, 2016
@tptee tptee deleted the feature/next branch August 8, 2016 23:26
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.

6 participants