next-offline is incompatible with next-compose-plugins

[hanford]

This issue has been reported a few times on the old Zeit slack, along with in some other places like #67

I'm fairly sure this problem a next-offline specific issue that we should either fix or find a work around

[NathanielHill]

I was one of those original reporters, but FWIW I haven't had any problems since using next-compose-plugins phase feature to disable next-offline in development.

[hanford]

@NathanielHill ahh, interesting. That'll be what I suggest until get to the bottom of it.

next-offline isn't really useful during development anyway ¯_(ツ)_/¯

[shawninder]

I'm having less luck with this problem myself. I made a repo dedicated to getting to the bottom of this, and although that repo reproduces the problem, the problem disappears when I reproduce it in test form. Anyone want to take a look and see if they can figure out what I'm doing wrong?

https://github.com/shawninder/next-manifest-sw-clash

[hanford]

@shawninder I should be able to take a look either tonight or this weekend. Thanks for creating the repo!

[hanford]

@NathanielHill can you share the snippet you have for disabling next-offline in next-compose-plugins during the development phase?

[NathanielHill]

Here's an example next.config.js:

const { PHASE_DEVELOPMENT_SERVER } = require('next/constants')                                                                                     
const nextOffline = require('next-offline')                                                                                                        
const path = require('path')                                                                                                                                                                                                                                                                       
const withPlugins = require('next-compose-plugins')                                                                                                
                                                                                                                                                   
module.exports = withPlugins(                                                                                                                      
  [                                                                                                                                                
    [nextOffline, ['!', PHASE_DEVELOPMENT_SERVER]]
  ],
  {
    webpack: config => {
      config.resolve.modules = [
        path.resolve('./src'),
        path.resolve('./public'),
        'node_modules'
      ]
      return config
    },
    // Other Next.js config here...
  }
)

[hanford]

@shawninder those tests are super helpful, I've actually gone ahead and cleaned them up a big and added them to this repo under the tests folder! Thank you so much!

Whats kind of blowing my mind is it seems that everything works fine when running locally

yarn build && yarn start

but not remotely (when deploying to now).

Running yarn dev is working as expected b/c both next-manifest and next-offline don't really do anything during development

[shawninder]

That version doesn't work for me locally either.

Just to rule out the obvious:

• Locally, your browser could be finding a service worker in it's application cache for localhost:3000, did you make sure to clear the site data when performing your manual test? (In Chrome: Dev tools, Application tab, Application->Clear Storage, [Clear site data] at the bottom)
• Do you have any environment variables set locally that could have an effect on the outcome? NODE_ENV and PHASE_DEVELOPMENT_SERVER come to mind here... Perhaps running things in a docker container could make things more reproducible...
• You could have files from old build lying around in your .next folder, did you try deleting that whole directory when performing your manual tests locally?

[NathanielHill]

@hanford Just FYI: I'm running greenkeeper.io on several of my repositories... and I've gotten dozens of emails this evening with all of your package updates being published. Several of the patch fixes failed to build which causes even more inbox noise. Is there anyway that you could do more local testing before publishing patch versions? :v:

[hanford]

@NathanielHill yeah my bad! I released a broken version and panicked

[hanford]

@shawninder what's crazy is that adding async to the webpack function makes all tests pass, while when it isn't there the only 3/4 are passing for me.

However, when having async webpack the actual library breaks and prevents the SW from being registered at all.

I am working in a totally clean environment without any globals/caches etc etc

[shawninder]

My understanding of why async fixes the tests is the following:

If another plugin assigns an async function to nextConfig.webpack (as next-manifest does), then the Next.js build tools can know to await that function before marking the build process itself as being finished.

However, if your own plugin replaces nextConfig.webpack by a function which is not async and returns a plain object (as opposed to a promise), then the Next.js build tools are unaware they should await anything before marking the build process as finished. In this case, the tests I wrote will go to the next step (checking the contents of the built files) too early.

So to fix the tests (and other scripts using the Next.js build tools programmatically), all plugins in the plugin chain need to make sure they support async nextConfig.webpack functions (just like how the Next.js build tools support it).

I would have thought that returning whatever nextConfig.webpack returns would have been enough since this would preserve promise-hood through the return chain, but what then proved insufficient, I tried async and that worked...

Perhaps the Next.js build tools have a way of checking whether a function is async but don't detect when it returns a promise? Anyways, maybe these ramblings will help you in your reflections as well.

[hanford]

I've created this repo

In the entry directory we have a script.js which we which we want to push into the applications code, and we do that with this block https://github.com/hanford/next-async-webpack-repro/blob/master/entry/plugin.js#L6 when webpack is an async function however, the console.log no longer appears.

That's why I had to revert https://github.com/hanford/next-offline/pull/73 b/c next-offline was no longer adding register-sw.js in the consumers code.

@shawninder

[hanford]

@NathanielHill is this still an issue for you? @MarkLyck said it's working for him, and it appears to be working okay for me in the test suite

https://github.com/hanford/next-offline/tree/master/tests

[NathanielHill]

@hanford, not having any problems here!

[NathanielHill]

Although, I always disable next-offline in dev mode for all my projects. Would you like me to test it enabled during development mode? That would be nice to remove that from my boilerplate. @hanford

[hanford]

that would be great @NathanielHill

During development next-offline is basically a noop and only registers this: https://github.com/hanford/next-offline/blob/master/service-worker.js

Let me know if you run into anything though!

[NathanielHill]

@hanford, I can confirm that the noop service-worker.js is built and dropped into .next/. However, this is a regression for me as I get a 404 and and SW registration failure in the console where before there was nothing (disable entirely during dev).

Perhaps it needs to be put in the build manifest or some such to be properly served in dev mode at /service-worker.js

[hanford]

ahh you know what @NathanielHill I don't think our dev service worker gets the same exact treatment as the service worker that we end up building for production.

Let's keep this open until that is fixed.

(it's probably pretty trivial, but it's lower priority than the documentation rewrite I'm working on for now 1.0 and now 2.0)

Thanks again for double checking! Hopefully won't be too long until we resolve that

[NathanielHill]

Sounds good. It looks like the noop service-worker.js file gets placed in .next/ and .next/server, but it's not added to any of the manifests. Looking forward to a fix on this, will be able to remove several lines of boilerplate from next.config.js across many projects. :+1:

[hanford]

@NathanielHill with next-compose-plugins are you invoking withOffline or are you relying on next-compose-plugins?

When my next.config.js looks like this https://gist.github.com/hanford/5375852981aa3db89abfdc7af46cacfe things appear to work as normal

Might be helpful if you could create a little repo where this issue is happening so i can pull it down and try to fix the use case.

Really appreciate the back and forth btw!

[NathanielHill]

I'm not invoking it, just passing to next-compose-plugins, perhaps that's my problem?

This is a typical setup for me:

https://gist.github.com/NathanielHill/f433b993e5ce0bee4d34e1573be36e29

[NathanielHill]

Appreciate you working on this @hanford :ok_hand:

[hanford]

I can create a next app with the above config and try to get things working — currently on holiday so I’ll try to report back in a week or so.

Happy New Years!

[hanford]

In the meantime @NathanielHill could you try invoking the next offline plugin when you pass it to compose .. similar to how I do it In the above test app? .. I’ve seen some other plugins require this too, not certain how/when next compose plugins invokes these 😅

[NathanielHill]

Invoking before passing doesn't make any difference for me. Again, the problem is not that the service-worker.js file is not generated, just that it's not served properly by the development server.

After a (development) build, I get the following two files:

.next/service-worker.js .next/server/service-worker.js

But I get 404s from http://localhost:3000/service-worker.js.

Everything works in production :+1:

[efleurine]

I saw many advices to disable next-offline during development phase. But in that case how do we test other features of SW?

[junibrosas]

@efleurine you can still test sw features by running your app in production mode locally. And thats npm run build && npm run start.

[elken]

Just to add (not a domain expert so feel free to correct the approach), but using a simple pipe function like (...ops) => ops.reduce((a, b) => (arg) => b(a(arg))) and merging all the options into a single object works for me.

Perhaps this could be adapted to provide a native way to create such a function.

gist

[hanford]

@elken I think that's a fine approach as well.