Skip to content
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.

Sign up for GitHub

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

Jump to bottom

react-app-rewired support react-scripts more than 2.1.2 version #344

Closed
wants to merge 1 commit into from
Closed

react-app-rewired support react-scripts more than 2.1.2 version #344

wants to merge 1 commit into from

Conversation

vdfor
Copy link

@vdfor vdfor commented Dec 25, 2018

No description provided.

@vdfor vdfor mentioned this pull request Dec 25, 2018
Closed
@AndyOGo
Copy link

AndyOGo commented Dec 26, 2018

@vdfor
Just switch to CRACO JS (crafted for CRA V2)
https://github.com/sharegate/craco

@marceloadsj
Copy link

marceloadsj commented Dec 26, 2018 •
edited
Loading

Maybe CRACO is broken too, haha! :-(

I think it's broken because CRA 2.1.2 merged webpack.config.dev with webpack.config.prod, using conditions to check dev/prod envs...

facebook/create-react-app#5722

I checked CRACO, it's using old approach too!
https://github.com/sharegate/craco/blob/master/packages/craco/lib/cra.js#L25
dilanx/craco#61

Can someone confirm this?

@AndyOGo
Copy link

AndyOGo commented Dec 26, 2018 •
edited
Loading

@vdfor
@marceloadsj
What a bummer, unfortunately this repo is not maintained any more.
So it may be easier to provide this PR and issue description at CRACO JS

@timarney
Copy link
Owner

If some people want to confirm this PR works I will merge.

AndyOGo
AndyOGo reviewed Dec 26, 2018
View reviewed changes
packages/react-app-rewired/scripts/build.js
const webpackConfigPath = paths.scriptVersion + "/config/webpack.config.prod";
const scriptPkg = require(paths.scriptVersion + "/package.json");

const isOldScript = !(scriptPkg && scriptPkg.version >= '2.1.2');
Copy link

Choose a reason for hiding this comment

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

I doubt this version comparison works for all semver features 😕

Copy link

Choose a reason for hiding this comment

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

We need to use the module to compare version
https://github.com/omichelsen/compare-versions
or
https://github.com/npm/node-semver

AndyOGo
AndyOGo reviewed Dec 26, 2018
View reviewed changes
packages/react-app-rewired/scripts/build.js

const isOldScript = !(scriptPkg && scriptPkg.version >= '2.1.2');

const webpackConfigPath = paths.scriptVersion + isOldScript ? "/config/webpack.config.prod" : "/config/webpack.config";
Copy link

Choose a reason for hiding this comment

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

I would rather use a try/catch block and require.resolve to detect which config file is available

@vdfor
Copy link
Author

vdfor commented Dec 27, 2018

@vdfor
@marceloadsj
What a bummer, unfortunately this repo is not maintained any more.
So it may be easier to provide this PR and issue description at CRACO JS

dilanx/craco#62
This pr will fix it at CRACO JS

tadatuta
tadatuta reviewed Dec 29, 2018
View reviewed changes
packages/react-app-rewired/scripts/build.js

const isOldScript = !(scriptPkg && scriptPkg.version >= '2.1.2');

const webpackConfigPath = paths.scriptVersion + isOldScript ? "/config/webpack.config.prod" : "/config/webpack.config";
Copy link

Choose a reason for hiding this comment

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

now it doesn't work :(

brackets are missing. should be

const webpackConfigPath = paths.scriptVersion + (isOldScript ? "/config/webpack.config.prod" : "/config/webpack.config");

or like this

const webpackConfigPathSuffix = isOldScript ? ".prod" : "";
const webpackConfigPath = paths.scriptVersion + "/config/webpack.config" + webpackConfigPathSuffix;

and the same for start

tiffon added a commit to tiffon/react-app-rewired that referenced this pull request Jan 2, 2019
@tiffon
Fix for CRA 2.1.2 timarney#343, builds on timarney#344
980be02
tiffon added a commit to tiffon/react-app-rewired that referenced this pull request Jan 2, 2019
@tiffon
Fix for CRA 2.1.2 timarney#343, builds on timarney#344
77e72d1
@tiffon tiffon mentioned this pull request Jan 2, 2019
Merged
@tangdw
Copy link

tangdw commented Jan 5, 2019

CRACO is broken too, switch to https://github.com/rescripts/rescripts

timarney pushed a commit that referenced this pull request Jan 6, 2019
@tiffon @timarney
Support webpack config factory (CRA v2.1.2) (#346)
db4ed32 
* react-app-rewired support react-scripts more than 2.1.2 version

* Fix for CRA 2.1.2 #343, builds on #344

* See about using node 6 and 8 in CI
@timarney
Copy link
Owner

timarney commented Jan 6, 2019

Closing this as this one has been merged.
#346

@timarney timarney closed this Jan 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hckhanh hckhanh hckhanh left review comments

@AndyOGo AndyOGo AndyOGo left review comments

@tadatuta tadatuta tadatuta left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@vdfor @AndyOGo @marceloadsj @timarney @tangdw @tadatuta @hckhanh