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

HMR example #39

Closed
wants to merge 10 commits into from
Closed

HMR example #39

wants to merge 10 commits into from

Conversation

ericclemmons
Copy link
Contributor

@ericclemmons ericclemmons commented Feb 12, 2017

This is a demo of HMR adds a hot-module-replacement example, but requires some changes, as seen in my example project:

https://github.com/ericclemmons/webpack-hot-server-example

  • app.js (and all of its dependencies) is where all changes should be made.
  • index.js is largely the HMR entry-point that gets started once upon compile, but should not change at all.
  • Because of this, and since HMR is a dev-mode feature, I was curious if the entry.main could point to backpack-core/scripts/hot.js or similar that would handle the boilerplate? The caveat is that you don't want the module that starts the server ever changing. (Check out index.js to see how this works. I do believe it's solvable though.)
  • I use my own start-server-webpack-plugin (meant for HMR, that's akin to reload-server-webpack-plugin), but it's simple enough to be within the dev script. You wouldn't need nodemon anymore.

hmr

@@ -0,0 +1,15 @@
{
"name": "backpack-examples-basic",
Copy link
Owner

Choose a reason for hiding this comment

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

should rename to backpack-examples-hmr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, just fixed with backpack-example-hot-module-replacement.

I can rename the whole thing to HMR, which is soooo much shorter!

@ericclemmons
Copy link
Contributor Author

@jaredpalmer Check out the demo in the comments above. It's typical of how we work on our projects (yay to not breaking socket connections & polling issues between restarts!), which works well, especially if you have webpack-dev-server in the mix, which can add 10s of seconds to start-up times.

@ericclemmons
Copy link
Contributor Author

If I were to make a hit list for this to reach parity with v0.0.8, it'd be:

  • Internalize start-server-webpack-plugin (one less dependency) as part of backpack dev.
  • Internalize the HMR aspect of index.js only for development. But what about .listen calls in the app?
  • Confirm that the existing chokidar behavior (add/remove/change/update) still works with Webpack watching.

@ericclemmons
Copy link
Contributor Author

Thinking on this more, would it be OK for users to not have to worry about writing .listen again?

app.listen(port, (err) => {
  if (err) {
    console.error(err)
  }

  if (__DEV__) { // webpack flags!
    console.log('> in development')
  }

  console.log(`> listening on port ${port}`)
})

It's sooo nice just writing:

import express from "express";

export default express().get("/", async (req, res) => {
  try {
    const thing = await Promise.resolve({ one: 'two' }) // async/await!
    return res.json({...thing, hello: 'world'}) // object-rest-spread!
  } catch (e) {
    return res.json({ error: e.message })
  }
});

@jaredpalmer
Copy link
Owner

this is awesome. we definitely need to think through .listen() issue, people have been using backpack for cli tools, scripts, and non-web stuff.

@ericclemmons
Copy link
Contributor Author

@jaredpalmer One of the techniques I've used in the past was checking the following:

  1. Does index.js export anything? If so, it may be a server.
  2. Does it have the typical express, koa, or http-server signature with .listen()? If so, call it.
  3. Does it have the typical hapi signature with .start()? If so, call it.
  4. Otherwise, it ain't a server.

@ericclemmons
Copy link
Contributor Author

@jaredpalmer For a separate issue, it would be good to have a contrived example of usage as a CLI tool or non-web stuff. I personally didn't consider it, but it makes sense!

@jaredpalmer
Copy link
Owner

jaredpalmer commented Feb 12, 2017

@ericclemmons do you think we need to document how HMR works (or doesn't work) with packages or services that do clustering such as pm2, throng, and forever?

@ericclemmons
Copy link
Contributor Author

hmr2

I'd like that the "friendly webpack plugin" or whatever the formatter is support the HMR stuff. I'm sure the author would accept a PR so there's less "noise".

@ericclemmons
Copy link
Contributor Author

Thanks for the tip on throng, I've never heard of it!

So, the dilemma with HMR is that if you make changes in the same file that has .listen, it'll try to start the server again because it re-runs that module.

Which is why these last few commits abstracted away the "server".

There are a couple ways we can tackle this:

  • Make hot / hmr an option to opt-in to.
  • Build a server.js or bundle it in main.js so people can use their own start script. (We can provide a default using throng if we want).

@malekjaroslav
Copy link

malekjaroslav commented Mar 29, 2018

Hi guys, I just wonder what happened with this functionality. I love using Backpack, but I came to the point where I need to connect to a Kafka cluster only once and not on every file save and it is impossible to achieve right now, isn't it?

@jaredpalmer
Copy link
Owner

@mxstbr may have some insights.

Backpack doesn't actually use HMR. It does a full restart managed by chokidar. However, we can easily add that functionality if ppl want it (my Razzle project does it).

@malekjaroslav
Copy link

I use Razzle for SSR client app, but this one is only a Node.js API and HMR would be great in order to achieve sigle connections to databases or straming platforms.

@malekjaroslav
Copy link

malekjaroslav commented Apr 5, 2018

@mxstbr @jaredpalmer I think adding that functionality would not hurt anyone and would be only a benefit, what do you think?

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants