closes #29

I also adapted the "Usage" section of the README to export an async function, as suggested by @ajitid in #29. I think this makes it clear enough that withRemoteRefresh is async, the types also represent that.

Problem

withRemoteRefresh of next-remote-refresh sometimes throws an error:

error - Failed to load next.config.js, see more info here https://nextjs.org/docs/messages/next-config-error
Error: Cannot destructure property 'port' of 'createServer(...).address(...)' as it is null.
    at async Object.loadConfig [as default] (file:///home/projects/nextjs-y1ndyw/node_modules/next/dist/server/config.js:69:36)
    at async NextServer.prepare (file:///home/projects/nextjs-y1ndyw/node_modules/next/dist/server/next.js:130:24)
    at async eval (file:///home/projects/nextjs-y1ndyw/node_modules/next/dist/cli/next-dev.js:343:17)
TypeError: Cannot read properties of null (reading 'close')
    at RoundRobinHandle.remove (https://nextjsy1ndyw-jwoi.w-credentialless.staticblitz.com/blitz.6ebe18b1e1db1833797e3e4eb6ff802c375ee61b.js:6:1290129)
    at https://nextjsy1ndyw-jwoi.w-credentialless.staticblitz.com/blitz.6ebe18b1e1db1833797e3e4eb6ff802c375ee61b.js:6:1425701
    at Map.forEach (<anonymous>)
    at removeHandlesForWorker (https://nextjsy1ndyw-jwoi.w-credentialless.staticblitz.com/blitz.6ebe18b1e1db1833797e3e4eb6ff802c375ee61b.js:6:1425682)
    at ChildProcess.<anonymous> (https://nextjsy1ndyw-jwoi.w-credentialless.staticblitz.com/blitz.6ebe18b1e1db1833797e3e4eb6ff802c375ee61b.js:6:1427512)
    at Object.onceWrapper (https://nextjsy1ndyw-jwoi.w-credentialless.staticblitz.com/blitz.6ebe18b1e1db1833797e3e4eb6ff802c375ee61b.js:6:152909)
    at EventEmitter.emit (https://nextjsy1ndyw-jwoi.w-credentialless.staticblitz.com/blitz.6ebe18b1e1db1833797e3e4eb6ff802c375ee61b.js:6:155574)
    at finish (https://nextjsy1ndyw-jwoi.w-credentialless.staticblitz.com/blitz.6ebe18b1e1db1833797e3e4eb6ff802c375ee61b.js:6:963993)
    at _0x19cf18 (https://nextjsy1ndyw-jwoi.w-credentialless.staticblitz.com/blitz.6ebe18b1e1db1833797e3e4eb6ff802c375ee61b.js:15:148934)
    at https://nextjsy1ndyw-jwoi.w-credentialless.staticblitz.com/blitz.6ebe18b1e1db1833797e3e4eb6ff802c375ee61b.js:15:148682

Minimal reproduction example: https://stackblitz.com/edit/nextjs-y1ndyw

I have used next-remote-refresh successfully in the past for my homepage project, but suddenly the error occurs.

While I was not able to find out which change in my project did introduce this problem, I think I figured out the underlying problem:
Here, next-remote-refresh creates a websocket server and immediately tries to read the address from it:

https://github.com/souporserious/next-remote-refresh/blob/f934f4883d2a0794b96ba37e23cc88e50cd9e624/index.js#L9

It is using WebSocketServer.address() of the package ws, which is invoking http.Server.address() from Node.js:

github.com/websockets/ws/lib/websocket-server.js#L129-L145

And here's the thing: as is stated in the Node.js net server.address() docs:

server.address() returns null before the 'listening' event has been emitted or after calling server.close().

Solution

Wait for the listening event before reading the port from the server.
I created a patch with pnpm which I successfully apply in my project, see below. It makes the "withConfig" function async unfortunately, but when using next.config.mjs and a Node version supporting top-level-await, this is not a huge issue (as you can see here: github.com/pkerschbaum/pkerschbaum-homepage/packages/apps/web/next.config.js#L50).

diff --git a/index.d.ts b/index.d.ts
index 292d05f918836563e1e5d671ae3046a0b257a1a0..943d1eaca68dcb4e9471e8d2e35aeaef2afa1374 100644
--- a/index.d.ts
+++ b/index.d.ts
@@ -6,4 +6,4 @@ interface PluginOptions {
   signal?: AbortSignal
 }
 
-export default function plugin (options: PluginOptions): (nextConfig: NextConfig) => NextConfig
+export default function plugin (options: PluginOptions): (nextConfig: NextConfig) => Promise<NextConfig>
diff --git a/index.js b/index.js
index 811c02988758a45975eec5352689b8496974368a..ca557ed5a15eef0e6c1d91ef6651300bd20499ae 100644
--- a/index.js
+++ b/index.js
@@ -3,10 +3,15 @@ const createServer = require('./server')
 module.exports = function plugin(options) {
   let port
 
-  return function withConfig(nextConfig = {}) {
+  return async function withConfig(nextConfig = {}) {
     if (process.env.NODE_ENV !== 'production') {
+      const server = createServer(options)
+      await new Promise((resolve, reject) => {
+        server.on('listening', resolve)
+        server.on('error', reject)
+      })
       if (port === undefined) {
-        ({ port } = createServer(options).address())
+        ({ port } = server.address())
       }
 
       if (nextConfig.env === undefined) {

Thanks for making this package, it's super useful!

I noticed that the auto refresh would scroll to the top of the page, so this just makes it stay at the same position.

I missed this while refactoring the hook in the last PR. I'm avoiding assigning a default value in the hook's arguments since it would case the useEffect to run every render.

Hi! Thanks for the library, it works great aside from some of the client side logic that I've fixed here.

I also fixed the type definitions. I'm not sure how they were working before, the import at the top of the file causes TS to parse it as a module which prevents it from creating new module definitions in the global namespace. I've split it into two files for index.d.ts and hooks.d.ts since I think those are easier to maintain.

The other issue I was seeing is that the router.replace call in the hook was only ever reloading the route that the hook was initialized with. I separated the websocket initialization step into its own useEffect hook that only runs once. I modified the existing useEffect to update every time router.asPath changes and made sure that the hook cleans up after itself with removeEventListener in the destructor.

I have read the readme mentioning about this might break in future version of Next.js. Next.js 12 was released today and it breaks. Is there a way to fix this?

Hi again and happy new year!

I was able to make some time to give this library some test coverage and threw together a couple of simple workflow jobs with GH Actions as well.

When you have the time, I wanted to bring your attention to a couple things

• 👍 or 👎 on the StandardJS ESLint config? The existing code didn't need any changes aside from the whitespace after the function name with their config.
• ~~I've added a signal prop to the server options that allows tests to clean up after themselves with an AbortSignal. This doesn't break Node 12 compatibility[^1] since it's optional and doesn't rely on any global objects.~~ I've realized that this is unnecessary if I modify createServer to simply return a handle to the websocket server instance
• the parsing of the filePath passed to the chokidar callback is buggy when the changed file doesn't share a common ancestor with the working directory.
  • I think it's already a bit problematic that working directory basename is preprended to the base path since it makes equality checks from shouldRefresh more difficult to write. I think a better solution would be to pass a cwd to chokidar and just forward the paths emittered from there directly
  • my unit tests actually struggle with this since they write files to the /tmp directory when my workspace directory is in my home folder, so I'd like to include a fix to this in this PR if that's ok
• file creation/deletion doesn't trigger a refresh when it probably should

[^1]: AbortController/AbortSignal only has native support since 14.17

• Implement next plugin that spawn http server on dev
• Rewrite server to use SSE using vanilla http module to reduce dependencies
• Rewrite useRemoteRefresh hook to use internal next.js EventSource, this allows us to trigger next.js loading UI by encoding specific payload in SSE
• Remove hardcoded port in both server & client by relying on auto generated port in server + publicRuntimeConfig in client

Removed

• bin/next-remote-refresh as it's no longer used. This is technically breaking but we're still < v1.x.x.
• router arg in shouldRefresh callback due to stale router issue. IMO it's better for user to use their own useRouter if needed

On server startup, next-remote-refresh logs all of the files matched in the paths glob(s) due to chokidar's default behavior of emitting add/addDir for all of the files it discovers. Setting this configuration to true lets us avoid a bunch of unnecessary console spam.

Trying to figure out a way to make setup easier by creating a plugin, ideally we can just do this:

const withRemoteRefresh = require('next-remote-refresh')({
  paths: ['path', 'to', 'files'],
  port: 4001,
})
module.exports = withRemoteRefresh()

Not sure this is viable though? Guessing that creating an additional server as a side-effect isn't a good idea 😅

This PR contains the aforementioned fixes to the paths created by the file watcher. I'm passing cwd to chokidar to make it return paths relative to process.cwd(). These paths are then sent as-is to any connected clients.

This is a breaking change in that the basename of the working directory is no longer prepended - I think this is necessary because the old strategy was incompatible with paths which share no common ancestor with the working directory. It also makes equality checks simpler on the front end (for me at least - in my case I currently have to strip out any leading ../ and check for a match with .endsWith()).

Just the start of this PR, not sure if it's viable or not. The current state of this is grabbing the wrong _app file. I don't know enough about NextJS internals for this, but thinking we can detect if an _app file is present and inject the hook or construct our own _app wrapper. An AST transform is probably better here, but wanted minimal performance impact if possible.

closes #6

Ideally, we should only need to set up the plugin and not worry about the hook. I'm thinking we can create a custom loader that injects the hook into the app:

// index.js
config.module.rules.unshift({
  test: /\.(js|jsx|ts|tsx)$/,
  use: [{ loader: path.resolve(__dirname, 'loader') }],
})

// loader.js
module.exports = async function (source) {
  const callback = this.async()
  if (this.resourcePath.includes('_app')) {
    // insert hook here
  }
  return callback(null, source)
}

I don't have much time for this right now so a good first issue if someone wants to take this!