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

Add support for a static folder #226

Closed
wants to merge 3 commits into from
Closed

Add support for a static folder #226

wants to merge 3 commits into from

Conversation

mxstbr
Copy link
Contributor

@mxstbr mxstbr commented Jul 26, 2016

It is a nice escape hatch, ref: #28

Verified by going through the entire flow (in all three states, i.e. node_modules, development and ejected) with npm start.

It is a nice escape hatch, ref: #28
@ghost ghost added the CLA Signed label Jul 26, 2016
@mxstbr mxstbr mentioned this pull request Jul 29, 2016
@gaearon
Copy link
Contributor

gaearon commented Jul 30, 2016

Can you please rebase this? I want to take a look this week.

@mxstbr
Copy link
Contributor Author

mxstbr commented Jul 30, 2016

Put it on my todo list, will do this evening!

@mxstbr
Copy link
Contributor Author

mxstbr commented Jul 30, 2016

Rebased!

@ghost ghost added the CLA Signed label Jul 30, 2016
@gaearon
Copy link
Contributor

gaearon commented Aug 8, 2016

What happens if I just reference /src/image.png in my HTML? If it works now we need to make sure it doesn’t because that would be confusing.

@mxstbr
Copy link
Contributor Author

mxstbr commented Aug 9, 2016

Don't have time to try it out at the moment I'm afraid!

@mikaelbr
Copy link

mikaelbr commented Aug 9, 2016

Interesting thread! I was just experiencing this "short coming" today while testing out create-react-app and I needed to load Apple touch icons. I just happened to solve this the exact same way (only calling it assets which I understand was an unpopular choice), and silly me didn't check to see if there already was an open PR. I think this would be a great addition.

@gaearon I tested out referencing the /src/logo.svg directly from index.html and that didn't work. Neither did creating an arbitrarily file, /bar/foo.svg, and that could not be referenced from the static files either.

As a maybe "happy accident", by calling this static, it works as expected when referencing the files from the index.html it feels natural both in build and start mode as one can refer to the JS, CSS and media files from the same location. I also verified that a folder media in the template/static directory, got merged with the build/static/ directory and not overwritten.

Now it might feel "magical" that you have to create the static directory yourself and users need to confer with documentation by own initiative to check "if something like this just happens to exist". I think the template should have this folder by default by using something like PNG favicons or something.

Having something like this:

static/favicon/favicon-16.png
static/favicon/favicon-32.png

And in index.html:

...
<title>React App</title>
<link rel="icon" href="static/favicon/favicon-16.png" sizes="16x16" type="image/png">
<link rel="icon" href="static/favicon/favicon-32.png" sizes="32x32" type="image/png">
...

This way it would be easy to understand how to use it, and that it even exists. Just wanted to chime in and managed to write an entire essay doing so.

@ghost ghost added the CLA Signed label Aug 9, 2016
@gaearon
Copy link
Contributor

gaearon commented Aug 9, 2016

@mikaelbr Haha thanks for feedback on this!

@sco
Copy link

sco commented Aug 10, 2016

For a Service Worker to control a site, it has to be placed at the root. I.e., a service worker at /static/sw.js could only control pages that started with /static. There are other systems like Google's Website Verification that work by looking for a file at the root of a domain.

Any concern with simply removing the to option from CopyWebpackPlugin(), so that the contents of static are copied directly into the output?

@mxstbr
Copy link
Contributor Author

mxstbr commented Aug 10, 2016

@sco that'd be confusing if you put files into static/image.png but then have to reference /image.png. It might also override over files that are in the root? (imagine putting an index.html into static/…)

@mikaelbr woah thanks!

@ghost ghost added the CLA Signed label Aug 10, 2016
@sco
Copy link

sco commented Aug 10, 2016

copy-webpack-plugin actually won't overwrite any previously staged files, by default.

I can see how it might be a little confusing, but I think @mikaelbr's suggestion of putting the favicons there, and referencing them from index.html, would help to illustrate it pretty clearly.

I suspect putting arbitrary files at the root of a project will be a fairly common need. (robots.txt comes to mind, for instance.)

@mikaelbr
Copy link

There might be several files that are required to be in the root, so there might be a need to do this as well. What is the possible downsides of copying files from root to root?

By just copying files it'll be easier to restrict what files to actually copy and reduce the build-directory, but it might be more more confusing; "why do this files work, but I can't find /some-dir/bar.svg." Are there any huge side-effects by transferring all extra static files, except build size?

@lacker
Copy link
Contributor

lacker commented Aug 10, 2016

If you have a backend to your app, serving arbitrary static files from the root url that your webserver serves generally is not a good idea, because then you can't use a regex in your nginx/apache config to decide whether to route things to your static file service mechanism or to your backend. You usually want to special-case a small number of files, like robots.txt, crossdomain.xml, sw.js, and so on. So I suggest we continue serving assets in /static/ from a url containing /static/.

Perhaps you could actually work around this quite easily if you don't have a backend, by proxying to localhost:3000/static. That will proxy localhost:3000/sw.js to localhost:3000/static/sw.js IIUC. If you do have a backend then this won't work, but if you do have a backend then you already have the problem that the default logic will proxy /sw.js to your backend. You might just want to serve a handful of static files from your existing backend in that case.

@tal
Copy link

tal commented Aug 29, 2016

Depends on whether the associated images in there would be passed though
webpack as well to have hashes added for cdn cache busting reasons.

I wouldn't mind building a pr for all this but have no clue if it's what
the maintainers want.
On Mon, Aug 29, 2016 at 4:01 AM Max Stoiber notifications@github.com
wrote:

I think it's much better to move index.html

This would be a much better solution if index.html wasn't passed through
webpack with the HTMLWebpackPlugin – but alas, it is. That'd only make
things more confusing if suddenly one single file in the static/ folder
was passed through webpack but all the others aren't.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#226 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABJ2igg8U4OqFJMyqeGmlDfRTZDG2BGks5qkpHhgaJpZM4JVdxr
.

@mxstbr
Copy link
Contributor Author

mxstbr commented Aug 29, 2016

Depends on whether the associated images in there would be passed though webpack as well to have hashes added for cdn cache busting reasons.

Nope, then they should go into the src folder next to the respective components. The whole point of having a static/ folder is that it's an escape hatch out of all of that behaviour.

@tal
Copy link

tal commented Aug 29, 2016

This is tangential to this PR, but if that's the case then index.html should be inside of src anyways.

And with that context I'd say that having static is a mistake that would lead to more confusion than it would solve. It gives 2 locations for things to be stored rather than just one. For those who need the more advanced behavior they can always eject and do themselves.

@davidascher
Copy link
Contributor

@tal I personally have no useful opinion on layout or even what should or shouldn't go through the webpack transformation.

On the topic of capabilities, though: I do think that it's good to have a fairly ambitious definition of what can be done without ejecting. So, in particular, serving favicons for all major OSes and as much of the progressive web app feature set as possible (good service workers defaults, etc.), should not require ejecting.

If those capabilities are accessible (but not required) in the default setup, then more people will implement them, and more webapps will be better webapps, which is good for users and good for the web.

@tal
Copy link

tal commented Aug 29, 2016

What i was meaning was that those favicon or icons of whatever sort for all
the OSes are specified by meta tags. Not convention. That means that they
should be cache busted, this probably means going though webpack in some
way.

On Mon, Aug 29, 2016 at 12:45 PM David Ascher notifications@github.com
wrote:

@tal https://github.com/tal I personally have no useful opinion on
layout or even what should or shouldn't go through the webpack
transformation.

On the topic of capabilities, though: I do think that it's good to have
a fairly ambitious definition of what can be done without ejecting. So, in
particular, serving favicons for all major OSes and as much of the
progressive web app feature set as possible (good service workers defaults,
etc.), should not require ejecting.

If those capabilities are accessible (but not required) in the default
setup, then more people will implement them, and more webapps will be
better webapps, which is good for users and good for the web.


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#226 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABJ2rwYSvWX7TlyHBe3QVKaCxlVFWQUks5qkwyggaJpZM4JVdxr
.

@ghost ghost added the CLA Signed label Aug 29, 2016
@SpaceK33z
Copy link
Contributor

@tal, in that case, maybe a package like webpack-manifest-plugin would be interesting. It works by adding this to your webpack loader config:

{
    test: /manifest.json$/,
    loader: 'file?name=manifest.json!web-app-manifest',
},

@gaearon
Copy link
Contributor

gaearon commented Sep 2, 2016

Sorry for the long turnaround on this.
I’ve been weighing pros and cons and I’m coming to realization that this is not the approach I want to take.

Instead, #428 should satisfy most use cases, and if not, we can address them on case by case basis (file issues!)

Please see #551 for more comments on this, justification for not supporting this, and additional documentation that I believe adequately explains how #428 works as well as the behavior I ultimately want to have.

Sorry 😥.

@gaearon gaearon closed this Sep 2, 2016
@gaearon gaearon reopened this Sep 6, 2016
@gaearon gaearon added this to the 0.5.0 milestone Sep 6, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 6, 2016

We got some more feedback and I know more about the use cases people have for this. I couldn't find any better alternative so let's do it in the next minor.

@ghost ghost added the CLA Signed label Sep 6, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 13, 2016

Thought some more about it. It won't work for non-relative roots (like GH Pages).

People will reference /static in their HTML and it will 404 after hosting.

Any ideas how to solve that?

Copy link
Contributor

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

/static won’t work on non-root hosting which is potentially confusing. I think we might need to come up with some other way of supporting this.

@kahneraja
Copy link

would love to help with this issue. it relates to the open graph meta tag issue i'm wanting resolve too.

what should i try?

@gaearon
Copy link
Contributor

gaearon commented Sep 22, 2016

I’m going to close this because I ended up with a different solution that I think addresses these concerns better: #703. Let me know what you think!

@gaearon gaearon closed this Sep 22, 2016
@gaearon gaearon deleted the static-folder branch September 22, 2016 19:14
@gaearon gaearon removed this from the 0.5.0 milestone Sep 23, 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.

@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

Successfully merging this pull request may close these issues.

None yet

10 participants