Skip to content
This repository has been archived by the owner on May 17, 2019. It is now read-only.

Change createBrowserHistory imports #250

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

ajbogh
Copy link

@ajbogh ajbogh commented Apr 26, 2019

The method of importing createBrowserHistory with import createBrowserHistory from 'history/createBrowserHistory' causes warning messages during testing. The message produced below includes an example of how to import the createBrowserHistory function. This change updates the code to be compatible with the intent of the message.

  console.error node_modules/history/warnAboutDeprecatedCJSRequire.js:17
    Warning: Please use `require("history").createBrowserHistory` instead of `require("history/createBrowserHistory")`. Support for the latter will be removed in the next major release.

The method of importing createBrowserHistory with `import createBrowserHistory from 'history/createBrowserHistory'` causes warning messages during testing. The message produced below includes an example of how to import the `createBrowserHistory` function. This change updates the code to be compatible with the intent of the message.

```
  console.error node_modules/history/warnAboutDeprecatedCJSRequire.js:17
    Warning: Please use `require("history").createBrowserHistory` instead of `require("history/createBrowserHistory")`. Support for the latter will be removed in the next major release.
```
@CLAassistant
Copy link

CLAassistant commented Apr 26, 2019

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@KevinGrandon KevinGrandon left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Copy link
Contributor

@KevinGrandon KevinGrandon left a comment

Choose a reason for hiding this comment

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

Hmm, looks like we have some CI failures to fix.

@AlexMSmithCA
Copy link
Member

Not sure if the failures are public. Here's what's failing:

From buildkite/fusion-plugin-react-router/eslint:

$ eslint . --ignore-path .gitignore
--
  | Warning: React version not specified in eslint-plugin-react settings. See https://github.com/yannickcr/eslint-plugin-react#configuration.
  |  
  | /fusion-plugin-react-router/src/plugin.js
  | 11:9  error  Replace `·createBrowserHistory·` with `createBrowserHistory`  prettier/prettier
  |  
  | ✖ 1 problem (1 error, 0 warnings)
  | 1 error and 0 warnings potentially fixable with the `--fix` option.

From buildkite/fusion-plugin-react-router/flowtype :

$ /fusion-plugin-react-router/node_modules/.bin/flow
--
  | Launching Flow server for /fusion-plugin-react-router
  | Spawned flow server (pid=29)
  | Logs will go to /tmp/flow/zSfusion-plugin-react-router.log
  | Monitor logs will go to /tmp/flow/zSfusion-plugin-react-router.monitor_log
  | Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/plugin.js:11:10
  |  
  | Cannot import createBrowserHistory because there is no createBrowserHistory
  | export in history.
  |  
  | 8│
  | 9│ import * as React from 'react';
  | 10│ import {Router as DefaultProvider} from 'react-router-dom';
  | 11│ import { createBrowserHistory } from 'history';
  | 12│
  | 13│ import {UniversalEventsToken} from 'fusion-plugin-universal-events';
  | 14│ import {createPlugin, createToken, html, unescape, memoize} from 'fusion-core';
  |  
  |  
  |  
  | Found 1 error
  | error Command failed with exit code 2.

The import method for history required an update to v4.9.0.
@codecov
Copy link

codecov bot commented Apr 27, 2019

Codecov Report

Merging #250 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #250   +/-   ##
=======================================
  Coverage   79.81%   79.81%           
=======================================
  Files          10       10           
  Lines         218      218           
  Branches       48       48           
=======================================
  Hits          174      174           
  Misses         30       30           
  Partials       14       14
Impacted Files Coverage Δ
src/plugin.js 85.24% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1558eda...bc7c3c0. Read the comment docs.

Removed spaces.
@ajbogh
Copy link
Author

ajbogh commented Apr 27, 2019

Updated. Hopefully it works!

@ajbogh
Copy link
Author

ajbogh commented May 8, 2019

@KevinGrandon Can someone double-check the build? I've updated the code but it's still failing. I can't access the details of the failures because the pages 404. Some help would be appreciated on this.

@KevinGrandon
Copy link
Contributor

@KevinGrandon Can someone double-check the build? I've updated the code but it's still failing. I can't access the details of the failures because the pages 404. Some help would be appreciated on this.

We will work on exposing the public page more easily, but for now you should be able to see the failing task and reproduce locally.

I was looking into the flow failure a bit for this and ended up submitting: flow-typed/flow-typed#3306

Trying those definitions out in: #251

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants