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

version 0.4.1 serves non-HTML files as text/html #573

Closed
donpark opened this issue Sep 4, 2016 · 28 comments
Closed

version 0.4.1 serves non-HTML files as text/html #573

donpark opened this issue Sep 4, 2016 · 28 comments
Milestone

Comments

@donpark
Copy link

donpark commented Sep 4, 2016

my index.html file needs to load some CSS and JS files directly which worked fined in previous releases but not with react-scripts version 0.4.1 because they are served with wrong content-type text/html.

These files cannot be loaded using import statements in JS files due to various issues caused by some obscure differences between two approaches.

@gaearon
Copy link
Contributor

gaearon commented Sep 4, 2016

Please see 0.4.0 release notes and description of the breaking change. This only worked before by accident and was never supported (for example it only worked in development but not in production). If you need this feature please raise an issue explaining why in detail, preferably with an example project. This was also mentioned in 0.4.0 release notes breaking change description. Thank you!

@gaearon
Copy link
Contributor

gaearon commented Sep 4, 2016

(You can find release notes either in changelog.md or on GH Releases page.)

@gaearon
Copy link
Contributor

gaearon commented Sep 4, 2016

As a workaround, you can upload those files to your server or serve them locally from another host. I understand this is not ideal, but to help you I would need to know what kind of incompatibilities don't let you import those files through the module system.

I'm also curious how you handled them in production. Did you specifically copy them as an additional build step? Did you manually bump versions in HTML file every time you updated them?

@gaearon
Copy link
Contributor

gaearon commented Sep 4, 2016

Relevant change for some context: #551

@donpark
Copy link
Author

donpark commented Sep 4, 2016

Although I've written many React apps, this is my first 'create-react-app' and it hasn't been deployed yet although I've gone as far as running production build in a docker container without problems before.

First trouble I had was with a jquery-based library, knob.js, which complained about jQuery missing although I've imported it just before. I backed tracked to 0.3.1 immediate to keep things moving so haven't explored why it wasn't able to find jQuery.

Sorry to hear this feature was accidental. No big deal. There is always the eject option. It's just that I like the convenience and wanted to defer ejecting as long as I can. :-)

@gaearon
Copy link
Contributor

gaearon commented Sep 4, 2016

I'm not saying you should necessarily eject. I'm saying the feature was broken and it we want to support it, we need to fully understand a range of use cases for it. So I'd like to ask you to share a code example that worked in 0.3.0 both for development and production but broke in 0.4.x so that I both understand your use case better, and can test a complete new solution if we add one for 1.0. If you can't share it, unfortunately, it will be less likely that we'll solve this problem, because we need to accumulate some use cases to come up with a proper solution.

@donpark
Copy link
Author

donpark commented Sep 4, 2016

Sadly, I can't share my project but I can point out which files I had trouble using import with:

  • jQuery (imported directly instead of npm module)
  • jQuery-Knob

If I can find time today, I'll retry 0.4.1 again to explore the import issue. I don't know how much trouble resource loading from index.html file is but I think it's something that's worthwhile to support since there are huge number and variety of CSS and JS libs out there and these sort of import-related issues will keep cropping up.

@cloudmu
Copy link
Contributor

cloudmu commented Sep 9, 2016

@donpark you can npm install jQuery as dependency, and then add the following in your index.js:

window.$ = window.jQuery=require('jquery');

or

import jquery from 'jquery';
window.$ = window.jQuery=jquery;

You can also import bootstrap, font-awesome, etc. See index.js in my starter kit to gather some idea. They all work well with create-react-app 0.4.1, and I think it's a cleaner approach than loading them from the index.html. @gaearon

@donpark
Copy link
Author

donpark commented Sep 9, 2016

Yeah, I did just that today with jQuery and jQuery-Knob to stay in good grace of react-scripts. In my case, both javascript library had npm versions so this solves my problem but others may not be as lucky.

I don't know what compelling benefits requiring webpack loader-based inclusions brings to create-react-app users and developers but I think it's a glaring flaw in an otherwise wonderful developer tool.

@gaearon
Copy link
Contributor

gaearon commented Sep 9, 2016

I don't know what compelling benefits requiring webpack loader-based inclusions brings to create-react-app users and developers but I think it's a glaring flaw in an otherwise wonderful developer tool.

Content hashing works correctly. If you update your jQuery plugin, an old version won't be accidentally cached by your users.

If you use code splitting, you can delay loading jQuery and plugin code until you actually load the component using it, without writing any complicated code to inject scripts dynamically. Thanks to Webpack.

Same for any other asset types. Images get correct hashes automatically.

If you link to an image, and that image gets deleted, you will get a compile time error rather than a 404 in the app.

There are numerous benefits 😉

@cloudmu
Copy link
Contributor

cloudmu commented Sep 9, 2016

I was thinking libs without npm versions may be the legit use cases for directly linking them from index.html. But just realized they also can/should be loaded via webpack. In my test the following index.js works just fine. (note jquery is not npm installed but rather saved as a 3rd-party lib outside of src)

import jquery from '../libs/jQuery/jquery-3.1.0.min.js';
window.$ = window.jQuery=jquery;

It works under both dev and prod.

@cloudmu
Copy link
Contributor

cloudmu commented Sep 9, 2016

Prior to CRA, I actually spent quite a bit of effort (with my own webpack and build scripts) making static assets linked in index.html to work under both dev and prod, due to some baseUrl related issues. I made it work but I would have still missed all the benefits @gaearon outlined.

So I like the philosophy (wisdom) of being really conservative when introducing "features" in CRA.

@sladkovm
Copy link

Hi, i think i stumbled upon the same issue, when using data loaders from 'd3-request' module (does React + D3.js qualifies as a good business-case for getting it fixed?).

In particular i did try { csv } and { text } and in both cases it returns index.html iso the content of actual files I wanted to load.

I happened to know that the d3-request was working OK with 0.2.3 CRA version and the example of it can be found in Jerome Cukier repo - https://github.com/jckr/example-react-app

Now, the practical question - how can I install the 0.2.3 CRA (or anything before the 0.4) directly, without simply cloning the above-mentioned Jerome's Cukier build and assume it as my baseline.

@donpark
Copy link
Author

donpark commented Sep 15, 2016

There is a possible downside to packing all scripts into one bundle which is that, if browsers evaluate script resources in parallel, then evaluation of scripts in a bundle may be slower than not including libraries like jQuery and D3 in the bundle.

My app in dev mode spends ~150ms loading then pauses for ~800ms in "Evaluate Script" state before running. Yes, it's ghastly. If jQuery and other libs like D3 were loaded separately then script evaluation time overall may drop. I say may because this have not been verified. I plan to test this theory later.

@gaearon
Copy link
Contributor

gaearon commented Sep 16, 2016

JavaScript is single-threaded so no, libraries are not evaluated in parallel if they are added separately.

@gaearon
Copy link
Contributor

gaearon commented Sep 16, 2016

I happened to know that the d3-request was working OK with 0.2.3 CRA version

It was not really "working". It happens to appear working in development mode but that file will be missing in the production build.

Now, the practical question - how can I install the 0.2.3 CRA (or anything before the 0.4) directly, without simply cloning the above-mentioned Jerome's Cukier build and assume it as my baseline.

I would rather recommend you to eject than to use an old version. Old versions have bugs ;-). But if you must, change the version of react-scripts in your package.json to any past version and run npm install.

@donpark
Copy link
Author

donpark commented Sep 16, 2016

JavaScript is single-threaded so no, libraries are not evaluated in parallel if they are added separately.

JavaScript is but JavaScript parsers, written in C/C++, are not. So we can't draw conclusions without proof. But at least one thing is certain, monolithic bundles cannot be parsed in parallel.

@cloudmu
Copy link
Contributor

cloudmu commented Sep 16, 2016

My app in dev mode spends ~150ms loading then pauses for ~800ms in "Evaluate Script" state before running

Very curious about these numbers under production mode. @donpark

@sladkovm
Copy link

Ok, thanks. I can't really follow the logic why the useful feature is in fact a bug, but for my humble purposes of trying quick and dirty React with D3 the JC's repo run in dev mode would do the trick.

btw, I've failed to regress the 4.1 version to 2.3 - got a log list if webpack complaints.

@gaearon
Copy link
Contributor

gaearon commented Sep 16, 2016

JavaScript is but JavaScript parsers, written in C/C++, are not. So we can't draw conclusions without proof. But at least one thing is certain, monolithic bundles cannot be parsed in parallel.

I don’t know about parsing, I thought you were talking about evaluation. But this is very tangential to this topic so if you’d like to discuss this, please open a separate issue.

My app in dev mode

Dev mode is slower btw, this is expected. Please use production mode for measuring performance.

Ok, thanks. I can't really follow the logic why the useful feature is in fact a bug

I was not referring to this feature. I’m saying I don’t recommend using old versions because they have (other) bugs which have since been fixed. Staying with 0.2.3 because this seemed to work is not a good strategy.

And as I explained in #573 (comment), it was not working in the first place. I believe I explained in that comment why this was not a “feature”, it only accidentally seemed to work, and only in development. It was broken in production.

We will consider supporting this use case in the future (this issue got a milestone 😉 ). Let’s keep the further discussion on topic.

@donpark
Copy link
Author

donpark commented Sep 16, 2016

please open a separate issue.

Will do once I get a clearer picture of what's going on and whether splitting up bundles will reduce evaluate script time.

We will consider supporting this use case in the future (this issue got a milestone 😉 ). Let’s keep the further discussion on topic.

Fantastic. Music to my ears. I'd close this issue but I'm not sure what the team's policy on OP issue closing.

@gaearon
Copy link
Contributor

gaearon commented Sep 16, 2016

I’d like to keep it open so I can later come back to all of them and make sure we have a way of addressing the requirements.

@gaearon gaearon modified the milestones: 0.5.0, 1.0.0 Sep 22, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 22, 2016

I drafted a proposal to solve this in #703. Unless we find some fatal flaws, it should come out in 0.5.0. Let me know what you think!

@gaearon
Copy link
Contributor

gaearon commented Sep 22, 2016

Closing as this is fixed, and will be released in 0.5.0.

@gaearon gaearon closed this as completed Sep 22, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 23, 2016

This is now supported in 0.5.0.

Read about using the new public folder.

See also migration instructions and breaking changes in 0.5.0.

@donpark
Copy link
Author

donpark commented Sep 23, 2016

@gaearon with following HTML:

<link rel="shortcut icon" href="%PUBLIC_URL%/favicon.ico">
<script src="%PUBLIC_URL%/js/jquery/jquery.slim.min.js"></script>

I am getting:

URIError: Failed to decode param '/%PUBLIC_URL%/js/jquery/jquery.slim.min.js'
    at decodeURIComponent (native)

So first instance of %PUBLIC_URL% worked but not the second. I suspect you forgot to add RegExp/gflag in stringreplace` call.

@gaearon
Copy link
Contributor

gaearon commented Sep 24, 2016

@donpark Fixed in 0.5.1, thanks.

@sladkovm
Copy link

Thanks! d3.csv import also works now.

@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants