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

importing images #39

Closed
tonis2 opened this issue Jul 19, 2015 · 34 comments
Closed

importing images #39

tonis2 opened this issue Jul 19, 2015 · 34 comments

Comments

@tonis2
Copy link

tonis2 commented Jul 19, 2015

Has anyone got importing images with webpack working? For me it seems to break server render.

@erikras
Copy link
Owner

erikras commented Jul 19, 2015

I've got it working, but only via css background-image: url('./dog.jpg') sort of stuff. I will investigate doing so in javascript.

@huangdawei
Copy link

I can't get it to work with server side rendering too, any progress?

@erikras
Copy link
Owner

erikras commented Jul 21, 2015

Right, that was the problem. I'd forgotten Because you can't just wrap it in an if (__CLIENT__) {} like you can with the stylesheets. I wasn't able to figure a way around that when I was looking. 😞

@huangdawei
Copy link

Thanks, it's a temporary solution, but not a good idea since the Server-side code is discarded and the images start to load only if the of JS files were loaded.

@huangdawei
Copy link

Maybe it's a problem of Babel.

@erikras
Copy link
Owner

erikras commented Jul 28, 2015

Insight from @leonli:


https://github.com/iam4x/isomorphic-flux-boilerplate/blob/master/app/utils/image-resolver.js

import imageResolver from 'utils/image-resolver'

let image;
// On browser just require() the image as usual
if (process.env.BROWSER) {
  image = require('images/logo.png');
}
else {
  image = imageResolver('images/logo.png');
}

...
render () {
  return (
    <img src={image} />
  );
}
...

@leonli
Copy link
Contributor

leonli commented Jul 28, 2015

Just a thought, and it's also related to #48
Instead of reading images and scss files from the memory of webpack stats, I would prefer the elegant way is to write the better loaders for sass-loader and the image-file-loader, turn on the feature of rendering both on browser and the nodejs, am I right?

@erikras
Copy link
Owner

erikras commented Jul 28, 2015

@leonli Possibly.

@leonli
Copy link
Contributor

leonli commented Jul 30, 2015

PR #66 sent. Implemented the solution I talked above. 😄

@NogsMPLS
Copy link

Still get issues with home page logo image rendering. The server markup isn't matching client markup, specifically because of the logo image.

image.assets[0] is localhost:3001/dist/undefined

@erikras
Copy link
Owner

erikras commented Jul 30, 2015

Good catch. I'm seeing that too. Will investigate.

@huangdawei
Copy link

May I ask why this is not a circular reference? Since the requireServerImage/Css requires webpack-stats.json and the compilation of webpack-stats.json based on the files include requireServerImage/Css.

React-hot-loader refreshes constantly in my repo if I require requireServerImage in the Components.

@erikras
Copy link
Owner

erikras commented Jul 31, 2015

@huangdawei It sort of is, but the circle is broken with the file system (it loads webpack-stats.json at runtime, but some version of the file must also exist at compile time. This is the source of #60, which makes you have to build twice initially.

erikras pushed a commit that referenced this issue Jul 31, 2015
* master: (21 commits)
  improved syntax for including styles
  fixed #77
  added apology for #60 in the README
  linted
  url-loader fix for images
  switched back to file-loader to avoid error from requireServerImage() fixes #39
  Works on mac now
  Added better-npm-run to allow setting env-vars on all platforms
  fixed cat pic on server render
  minor grammar and js style tweak to README
  added url-loader to production webpack, too
  merged PR #67. lint-corrected merge. added obligatory cat pic. changed to use url-loader to encode small images.
  made pass lint
  incorporated PR #67 and added some comments to promise chaining in createTransitionHook()
  util import should be in single line.
  correct the , to , sorry for my keyboard.
  remove a space :P
  update README.
  correct the util error :P
  add the image server render example to the logo.png.
  ...
erikras pushed a commit that referenced this issue Jul 31, 2015
* master: (32 commits)
  fixed npm script problem and upgraded redux-form
  added form support with redux-form
  npm knows how to prefix binaries
  oops, left in debugger call
  improved syntax for including styles
  fixed #77
  added apology for #60 in the README
  linted
  url-loader fix for images
  switched back to file-loader to avoid error from requireServerImage() fixes #39
  Works on mac now
  Added better-npm-run to allow setting env-vars on all platforms
  fixed cat pic on server render
  minor grammar and js style tweak to README
  added url-loader to production webpack, too
  merged PR #67. lint-corrected merge. added obligatory cat pic. changed to use url-loader to encode small images.
  made pass lint
  incorporated PR #67 and added some comments to promise chaining in createTransitionHook()
  work on forms
  work on forms
  ...
@catamphetamine
Copy link
Contributor

I am also having circular builds in my project (but not having circular builds in the example).
For me, this example is broken. If I put some nonsense into src/client.js it doesn't output any errors:

var testing = require('testing.json')

But if I do the same in my project's entry point it immediately says that testing.json is not found.

@catamphetamine
Copy link
Contributor

My solition to this issue:
https://github.com/halt-hammerzeit/cinema/blob/master/code/server/webpack.js

And it eliminates the initial build issue:
#60

load is a function from the global variable on the server (initialized at startup)
https://github.com/halt-hammerzeit/cinema/blob/master/code/server/libraries/utility.js

Still webpack outputs the green stats chart two times in a row - currently don't know why is the second time.

@catamphetamine
Copy link
Contributor

Or one could write require('image') without differentiating between the client and the server and instrument all server require calls like coffeescript does through require("coffeescript/register").
I guess that would be the right solution.

@catamphetamine
Copy link
Contributor

@catamphetamine
Copy link
Contributor

Ok, I made these require() hooks work and now I just do that:

import picture from './cat.jpg'

and it works both on the client and on the server.

I refactored the relevant code into a separate npm module in case someone needs that:
https://github.com/halt-hammerzeit/webpack-isomorphic-tools

@erikras
Copy link
Owner

erikras commented Aug 10, 2015

That looks awesome!! Can I get a PR to implement it here?

@catamphetamine
Copy link
Contributor

@erikras yeah, I think, I might do that. maybe in a week or two. cheers

@erikras
Copy link
Owner

erikras commented Aug 10, 2015

It would certainly warrant a place on the list of libraries making this project awesome in the README.

@catamphetamine
Copy link
Contributor

@erikras oh, so tempting) okay, I'm on it

@catamphetamine
Copy link
Contributor

@erikras So, I managed to integrate the two things together.
Check it out:
#108

I checked it on my machine and it seems to be working.

By the way, http://127.0.0.1:3000/survey doesn't work on my machine, looks like there is an unrelated bug

@erikras
Copy link
Owner

erikras commented Aug 10, 2015

The survey thing was a package.json problem. A pull should fix.

@catamphetamine
Copy link
Contributor

One little clarifying comment:

We could have wrote it like this (and left it like this)

import picture from './cat.jpg'

And it worked fine but if you changed the required file somehow it wouldn't update (the hash would be recalculated for the required file by webpack because it watches all the assets and rereuns the build process, and stats.json would be updated and re-loaded inside the express rendering middleware because of the if (__DEVELOPMENT__) block).
That's why I changed all asset imports to require()s and moved them inside render() blocks - this way I can flush require() caches in the if (__DEVELOPMENT__) block too, and such assets will be refreshed the next time the developer refreshes the page. (one can try this out say by editing an image file and inserting a random character somewhere in the middle therefore partially corrupting it. and then refreshing the webpage in development mode - the image on the page will also be corrupted because it will be re-require()d)

@catamphetamine
Copy link
Contributor

A follow-up, for everyone's information
https://github.com/halt-hammerzeit/webpack-isomorphic-tools#require-vs-import

@brendanmh
Copy link

How do you get this to work using universal-webpack?

@catamphetamine
Copy link
Contributor

@brendanmh by studying universal-webpack readme and the example project linked in the readme.

@brendanmh
Copy link

@halt-hammerzeit can you give me a hint as to what part of the readme is relevant? I have been following the readme and have many other things working, but not being able to import images and css from my components.

(Sidenote: I am also looking for a work-around for undefined window)

@catamphetamine
Copy link
Contributor

@brendanmh the most relevant parts of the readme are the first ones.

@brendanmh
Copy link

The advertisement or motivation section?

@catamphetamine
Copy link
Contributor

Oh, better look somewhere in the middle then.

@brendanmh
Copy link

Thanks for getting back to me so quickly. I trust that this exchange will be useful for other devs struggling with this problem.

@catamphetamine
Copy link
Contributor

By the way, when you're finished setting up the whole thing you can write some sort of a detailed blog post explaining the process so that it "will be useful for other devs struggling with this problem".

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

No branches or pull requests

7 participants