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

enable manifest plugin on dev #4014

Merged

Conversation

viankakrisna
Copy link
Contributor

needed this so i can query http://localhost:3000/asset-manifest.json for available assets to use while developing with a CMS

@iansu
Copy link
Contributor

iansu commented Feb 12, 2018

This seems useful. I know some people have requested the ability to remove hashes from filenames (for example: #3855). This might help in some of those situations too.

Is there any reason to not enable this in production as well as dev?

@viankakrisna
Copy link
Contributor Author

@iansu
Copy link
Contributor

iansu commented Feb 12, 2018

In that case, is there a reason this wasn't enabled in dev before? Any downsides?

@viankakrisna
Copy link
Contributor Author

hmm, looking through the history, there wasn't any discussion of enabling this in dev. This is the related issue for adding it in prod #600.

It might slow down recompilation on dev with lots of generated assets as it is mapping and reducing the webpack's stats object https://github.com/danethurber/webpack-manifest-plugin/blob/master/lib/plugin.js but I think it's negligible because the user usually has lots of sources but a minimal number of generated assets, and it's only working on the paths instead of the content.

another thing to add to the discussion is this would play well with #1588 once it's merged, as it will improve the use case of adding react to a CMS / existing application.

@iansu
Copy link
Contributor

iansu commented Feb 13, 2018

This looks good to me then.

@viankakrisna
Copy link
Contributor Author

@gaearon @Timer what do you think?

Copy link

@langpavel langpavel left a comment

Choose a reason for hiding this comment

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

I just create exactly same change in ejected app 👍

@Timer
Copy link
Contributor

Timer commented Feb 26, 2018

I don't see a problem with this, does it have speed implications or bugs on rebuilds?

@langpavel
Copy link

langpavel commented Feb 26, 2018

does it have speed implications

Minimal, there is no reason for this plugin to slow down build I believe

@langpavel
Copy link

langpavel commented Feb 26, 2018

BTW Is there an issue for tracking Webpack 4 upgrade?

@Timer
Copy link
Contributor

Timer commented Feb 26, 2018

#4077

@bugzpodder bugzpodder added this to the 2.0.0 milestone May 31, 2018
@bugzpodder bugzpodder merged commit 76ef9fa into facebook:next May 31, 2018
@Timer
Copy link
Contributor

Timer commented May 31, 2018

Did we follow up about the speed implications of this plugin?
I'm curious what the output of this is, and we probably need documentation explaining how it's to be used/that it exists instead of relying on someone searching pull requests.

@Timer
Copy link
Contributor

Timer commented May 31, 2018

Also, this issue only got a few thumbs up -- this seems pretty use case specific and I'm not sure if it has excellent value now that assets are being aggressively split; who's to say what needs to be included on initial page load?

I think this is hiding a much larger problem, which is we have to solve for the developing with a out-of-band-system use case, instead of adding hacks like these.

@bugzpodder
Copy link

bugzpodder commented May 31, 2018

Sorry about that, the comments in this PR seemed to suggest that people wanted this plugin and I tested this in my largish build and didn't seem to incur any performance penality or rebuilding bugs.
@iansu @viankakrisna do you have answers to @Timer's questions below? If there is concerns about usability/usefulness we should either address it via docs or leave the plugin out.

@Timer
Copy link
Contributor

Timer commented May 31, 2018

Nothing to be sorry for 😄, just asking questions -- I just want to make sure this gets documented if it's useful or removed if we can solve it with a larger change.

On another note, this is already enabled in production so I'm not sure if it's worth being dogmatic about for development.

@viankakrisna
Copy link
Contributor Author

Did we follow up about the speed implications of this plugin?

as per comments, it didn't seem to affect the performance much. we might need to add a script that shows perf difference between branches to have the exact numbers.

I'm curious what the output of this is, and we probably need documentation explaining how it's to be used/that it exists instead of relying on someone searching pull requests.

who's to say what needs to be included on initial page load?

this a result of querying http://localhost:3000/asset-manifest.json with an app that has one import() call

{
  "main.js": "/static/js/main.chunk.js",
  "main.js.map": "/static/js/main.chunk.js.map",
  "static/js/0.chunk.js": "/static/js/0.chunk.js",
  "static/js/0.chunk.js.map": "/static/js/0.chunk.js.map",
  "runtime~main.js": "/static/js/bundle.js",
  "runtime~main.js.map": "/static/js/bundle.js.map",
  "vendors.js": "/static/js/vendors.chunk.js",
  "vendors.js.map": "/static/js/vendors.chunk.js.map",
  "static/media/logo.svg": "/static/media/logo.5d5d9eef.svg",
  "index.html": "/index.html"
}

I agree this needs a documentation, but ideally with #1588 to avoid writing a half finished solution for integration with existing app / backend / cms

the main files has named keys and the dynamic import chunk + other assets has static/ prefix, i know it's not ideal but at least tools can process this output easily. Maybe we can namespace the main files for easier processing.

Also, this issue only got a few thumbs up -- this seems pretty use case specific and I'm not sure if it has excellent value now that assets are being aggressively split;

I think this is hiding a much larger problem, which is we have to solve for the developing with a out-of-band-system use case, instead of adding hacks like these.

This might only got a few thumbs up, but we have several issues (#1678 #3855 #4210 #4468 #4133 #3984 #3984) that can be solved by supporting this and #1588

I'd say that this is a first step of supporting these use cases (i know it's closed in #1678 and @gaearon said that we don't support this use case, but i feel it's a missed opportunity, seeing the issues listed above is opened more and more).

Also, i'm not sure if other tools has easy integration with backend, and i'm already familiar with how CRA works and too lazy to reconfigure these other tools 😄

zmitry pushed a commit to zmitry/create-react-app that referenced this pull request Sep 30, 2018
…ugin-on-dev

enable manifest plugin on dev
@lock lock bot locked and limited conversation to collaborators Jan 19, 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.

6 participants