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.

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

POC of providing a watching build mode that writes to the disk in development #1616

Closed
wants to merge 7 commits into from

Conversation

viankakrisna
Copy link
Contributor

@viankakrisna viankakrisna commented Feb 22, 2017

POC for #1070

  • Watching build mode by running npm run watch
  • Add react-scripts/scripts/watch.js and react-scripts/config/webpack.config.watch.js
  • Add prefix to the generated build files so we know that it's a development build. example: development.main.hash321.js
  • Cleans the build folder every compile, if the asset hash changed. So we don't end up with stale files in there. using packages/react-dev-utils/cleanBuildFolder.js
  • Prompts if the use case for using this mode is fulfilled at the start of the script, so the user won't accidentally using this mode if what they building doesn't need it.

@Timer
Copy link
Contributor

Timer commented Feb 22, 2017

This sure seems like a lot of duplicated code, can't we reuse the existing start and pass an argument to it that switches some functionality?

@gaearon
Copy link
Contributor

gaearon commented Feb 22, 2017

Also now that we use Webpack 2 we should probably start using its env feature.

@viankakrisna
Copy link
Contributor Author

viankakrisna commented Feb 22, 2017

@Timer Rather than that i'm thinking about extracting duplicated code and move it to react-dev-utils.
In my opinion flags are harder to maintain. The problem with start is there are webpackDevServer specific codes there which this script don't use. So rather than making it one giant mess, it's better to extract it to different files.

@gaearon

Also now that we use Webpack 2 we should probably start using its env feature.

Not quite familiar with this. Any links?

@gaearon
Copy link
Contributor

gaearon commented Feb 24, 2017

Not quite familiar with this. Any links?

https://gist.github.com/sokra/27b24881210b56bbaff7#configuration

@viankakrisna
Copy link
Contributor Author

Hmm, so we can call the config with arguments passed on. After reading this webpack/webpack-dev-server#62 (comment)

If we reuse start.js and emit the files there, we get the best of both world. A dev server that writes to disk. Problem is, it would still includes require.resolve('react-dev-utils/webpackHotDevClient'), and the generated file won't be hashed. And there are a lot of webpack-dev-server specific codes in there which the watch mode doesn't need.

If we reuse build.js, it's hardcoded to production build. And it includes specific instruction for deployment. Which the watching development mode doesn't need (or even suggest)

I think in watch.js it's more clearer what it's doing, a watch mode that generated development builds. Also it includes specific instruction for why they shouldn't use this mode.

Maybe we can merge the webpack config to one file first, then it would be easier to the caller script to make the switches.

For now, i think it's better to make the scripts focused, and extract duplicated code into their own modules.

@viankakrisna viankakrisna changed the title Provide a watching build mode that writes to the disk in development POC of providing a watching build mode that writes to the disk in development Mar 1, 2017

// Input: 1024, 2048
// Output: "(+1 KB)"
module.exports = function getDifferenceLabel(currentSize, previousSize) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you send a separate PR moving this into react-dev-utils?

var gzipSize = require('gzip-size').sync;
var paths = require('../config/paths');
// Print a detailed summary of build files.
module.exports = function printFileSizes(stats, previousSizeMap) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In fact let's move this whole thing into react-dev-utils (in a separate PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok!

@viankakrisna
Copy link
Contributor Author

this poc is broken after #1726 will refactor to use the changes in #1726

@viankakrisna viankakrisna changed the title POC of providing a watching build mode that writes to the disk in development [WIP] POC of providing a watching build mode that writes to the disk in development Mar 6, 2017
@viankakrisna viankakrisna changed the title [WIP] POC of providing a watching build mode that writes to the disk in development POC of providing a watching build mode that writes to the disk in development Mar 7, 2017
@viankakrisna
Copy link
Contributor Author

@gaearon @Timer is it preferable to extract duplicated webpack configs to webpack.config.common?

@Timer
Copy link
Contributor

Timer commented Mar 7, 2017

I believe it has come up before and we're unsure if we want to leave the users with the complexity of such a solution. You can ask Dan, however.

I believe we can now turn both webpack configs into one configuration using the env feature as explained by @gaearon.

@Timer
Copy link
Contributor

Timer commented Mar 7, 2017

I'm still not convinced, however, that this is necessary. I can't think of a situation where you can't use the dev-server, nor can I can imagine any reason why you wouldn't want to.

I have a feeling this will translate into people shipping bundled code from the watch feature, which may not include all the production optimizations (so you may still debug). If it matches the build operation, why do it at all? Everything is minified/you lose your env contexts/React is stripped of all its dev code.

And if it matches the start command, use the dev-server. 😛

@viankakrisna
Copy link
Contributor Author

viankakrisna commented Mar 7, 2017

@Timer what made me convinced that we need this is sometimes we cannot isolate the app itself with codes from the backend. And the story of react is how it integrates well with current code base smoothly.

Right now, there's much to be done for people working with non single page app setup with Create React App (usually figuring how to make the dev server working with their backend in like #1678 #1525). And they need to learn / know about it. It's not obvious, and takes time to master it.

The watching development mode should make everything easier because they just need to read the manifests, and then the generated code will be included similar to how the production behaves. So running npm run build is just an additional step to make the code suited for production. (We can also print this instruction in the watch mode).

I've made the code to explicitly named the files by their NODE_ENV setup, so I think shipping development bundle could be mitigated just by seeing what file is being shipped by its name.

I like how create-react-app attracts newcomers, and I don't want them to learn hacking their devServer to work with their current app just to use react in some part of their app. :)

@Timer
Copy link
Contributor

Timer commented Mar 7, 2017

cannot isolate the app itself with codes from the backend

In what regard? Isn't this exactly what the proxy property goes to achieve (so you don't need to)?

@viankakrisna
Copy link
Contributor Author

The proxy only serve json files, while in one of my setup I need to process img that is coming from my backend (without a proper cross origin headers). In production it will be fine, but in development it is such a pain to work with.

In another project, I don't need any index.html, because the structure is already spit out by the backend, React needs only to work with these structures (mount them in some places). So a simple includes of bundled development scripts will make the development easier to reason with, and more aligned with how the production should behaves.

In general, dev server made the development unaligned with production in these situations.

@viankakrisna
Copy link
Contributor Author

viankakrisna commented Mar 7, 2017

What I'm hoping we can achieve with this PR is Create React App can be a tool for integrating React with the currently running application. I know that there are alternatives that I can use. But I really think that the official way should also support integration easily because of how React works.

@cr101
Copy link
Contributor

cr101 commented Mar 7, 2017

@viankakrisna I totally agree with you. I'm still trying to figure out how to build a React WordPress theme using a coupled architecture and CRA. I hope that this PR can simplify things.

@viankakrisna
Copy link
Contributor Author

Currently i'm dog fooding it for my own projects on https://github.com/viankakrisna/create-react-app-extra. So anyone that wants to try it feel free to install it (and post issues if you found bugs).

@viankakrisna
Copy link
Contributor Author

viankakrisna commented Apr 20, 2017

I think #1994 & #1588 is a better solution rather than this.

@react-scripts-dangerous

Hello! I'm a bot that helps facilitate testing pull requests.

Your pull request (commit b20073df6a778b1e7e6c0db694ef6e9aa35343a3) has been released on npm for testing purposes.

npm i react-scripts-dangerous@1.0.15-b20073d.0
# or
yarn add react-scripts-dangerous@1.0.15-b20073d.0
# or
create-react-app --scripts-version=react-scripts-dangerous@1.0.15-b20073d.0 folder/

Note that the package has not been reviewed or vetted by the maintainers. Only install it at your own risk!

Thanks for your contribution!

@Nargonath
Copy link

@viankakrisna What is the current state of this PR? Is it going to be dropped down? I looked at the other PRs you linked to and AFAICT it doesn't help in the use case of browser extension development. This kind of development requires you to write a JSON manifest which links to JS scripts on disk for the browser to load hence I don't think we can use solution which redirects requests using proxing or such. I may have misunderstood the solutions suggested over there so please feel free to tell me in that case.

@viankakrisna
Copy link
Contributor Author

viankakrisna commented Apr 16, 2018

@Nargonath I think this PR is too old to be updated 😄 we might get a better luck after webpack 4 PR to see if the maintainers wanted to support these use cases. Meanwhile, I think you can add these changes to your ejected / forked react-scripts.

@Nargonath
Copy link

Nargonath commented Apr 16, 2018

@viankakrisna Alright, thank you for your answer. I don't want to eject so I'll just find a workaround and wait for future development on this.

By the way why not close the PR if it is not relevant anymore?

@viankakrisna
Copy link
Contributor Author

By the way why not close the PR if it is not relevant anymore?

i keep it open as an epitaph of my hopes and dreams for this feature to be baked in... 😄 actually i think i can update the branch to be relevant with the current changes, just need some time to do it.

@Nargonath
Copy link

@viankakrisna Fair enough, I understand. 😃 Feel free to ask if you need some help with that. I may not have that much time either but I'll try to help if I can. 😉

@Nargonath
Copy link

For those that would end up here looking for a solution, I released an npm package that allow to write dev build to the disk based on some discussions I've seen in this PR or other issues: https://www.npmjs.com/package/cra-build-watch. It works for my use case but I tried to make it general enough by accepting some arguments to please other people use case.

It is just meant as a temporary workaround until this moves forward and we can have the feature built-in.

Feel free to give me feedbacks on the github repo if there are stuff to improve or even if it works for you. 😉

@rebornix
Copy link

rebornix commented May 24, 2018

@viankakrisna this is my very first time using Create React App and immediately ran into this issue of CRA lacking watch mode with files saved on disk. Your solution helps almost perfectly for my use case, just want to say thank you, great work!

@Nargonath
Copy link

@rebornix Not sure if you were talking about what I did but if that's the case I'm glad I could help. Thanks for letting me know.

@stale
Copy link

stale bot commented Nov 2, 2018

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 7 days if no further activity occurs.

@stale stale bot added the stale label Nov 2, 2018
@Timer Timer closed this Nov 2, 2018
@lock lock bot locked and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants