Skip to content

Added WRITE_TO_DISK environment variable. Fixes #1070 #6144

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ioloie
Copy link

@ioloie ioloie commented Jan 7, 2019

This PR will allow us to pass the writeToDisk option to webpack-dev-middleware using a new environment variable WRITE_TO_DISK.

The new option addresses #1070 and use cases such as creating browser extensions.

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

Copy link

@Nargonath Nargonath left a comment

Choose a reason for hiding this comment

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

Perhaps it would be nice to allow a destination override instead of the default dist folder by adding another new environment variable. What do you think?

@rodcorsi
Copy link

rodcorsi commented Jan 9, 2019

@Nargonath @ioloie my 2 cents, as you can see on the documentation of webpack-dev-server, writeToDisk accept a boolean or a function returning the path, you could make WRITE_TO_DISK to have the same behavior.

WRITE_TO_DISK=true -> use default folder (dist)
WRITE_TO_DISK=<custom path> -> use the path specified

@ioloie
Copy link
Author

ioloie commented Jan 9, 2019

This PR has a shortcoming which is that the public folder isn't written to disk with each update.

Static files are loaded by the middleware upon receiving a HTTP request for the file. This means that this won't fix #1070 in it's entirety by itself. But it does put us in a much more manageable position since it's straightforward to use something like npm-run-all with cpx to look after the static files:

  "scripts": {
    "watch:react": "react-scripts start",
    "watch:static": "cpx \"public/**/*.*\" dist --watch",
    "build": "react-scripts build",
    "start": "npm-run-all -p watch:* ",
    "test": "react-scripts test",
    "eject": "react-scripts eject"
  }

@Nargonath
Copy link

@ioloie I agree that is not the best solution yet but it still is better than the current state where you have to rely on external tools: https://github.com/Nargonath/cra-build-watch

@ioloie
Copy link
Author

ioloie commented Jan 9, 2019

The writeToDisk function doesn't return a filePath. It accepts one as it's only argument.
It's purpose is to filter which files should be written to disk at all, returning either true or false.

To support custom output paths would mean making Webpack's output.path configurable which is a separate debate.

@Nargonath
Copy link

@ioloie Fair enough, LTGM. In the end I feel CRA should do the same that vue-cli did to allow customization of the different configs.

@stale
Copy link

stale bot commented Feb 8, 2019

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

@stale stale bot added the stale label Feb 8, 2019
@Nargonath
Copy link

Well we are just waiting for someone to merge it IIRC. 😃

@stale stale bot removed the stale label Feb 10, 2019
@stale
Copy link

stale bot commented Mar 12, 2019

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

@stale stale bot added the stale label Mar 12, 2019
@Nargonath
Copy link

The interest in the PR is still here, we just need it to be merged or someone to articulate his feedbacks if not suitable for merge already so we can move forward with it.

@stale stale bot removed the stale label Mar 12, 2019
@ifokeev
Copy link

ifokeev commented Mar 14, 2019

need this also

@animesh1987
Copy link

animesh1987 commented Mar 24, 2019

Also in need for this. Thanks.

@stale
Copy link

stale bot commented Apr 24, 2019

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

@stale stale bot added the stale label Apr 24, 2019
@Nargonath
Copy link

Sorry stale bot but no. 😛

@stale stale bot removed the stale label Apr 24, 2019
@skurfuerst
Copy link

Hey,

any news to this PR? Would be awesome to have this merged!

All the best,
Sebastian

@bugzpodder bugzpodder added this to the 3.x milestone Aug 11, 2019
@ryanwarsaw
Copy link

It would be great to have this merged, would make chrome extension development much easier.

@iansu iansu modified the milestones: 3.x, 3.2 Sep 27, 2019
@ianschmitz ianschmitz modified the milestones: 3.3, 3.4 Dec 5, 2019
@iansu iansu modified the milestones: 3.4, 3.5 Feb 14, 2020
@Janaka-Steph
Copy link

Pleaaase, merge this PR!

Removed unnecessary false

Co-authored-by: Andrew Hyndman <[email protected]>
@RehanSaeed
Copy link

RehanSaeed commented May 20, 2020

This would be really nice for building Chrome/Edge extensions.

@eddiemonge
Copy link
Contributor

doesn't this still suffer from the contents of /public not being written?

@EricRabil
Copy link

Just wanna say this seems to be working fine for me. I've got an npm package @erics-forks/react-scripts that is [email protected] with this PR merged. I'll do my best to keep it updated until this merges.

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

Successfully merging this pull request may close these issues.