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

next-offline is incompatible with next-compose-plugins #69

Open
hanford opened this issue Oct 31, 2018 · 35 comments
Open

next-offline is incompatible with next-compose-plugins #69

hanford opened this issue Oct 31, 2018 · 35 comments
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@hanford
Copy link
Owner

hanford commented Oct 31, 2018

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
Copy link

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
Copy link
Owner Author

hanford commented Nov 3, 2018

@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
Copy link
Contributor

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
Copy link
Owner Author

hanford commented Nov 8, 2018

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

@hanford
Copy link
Owner Author

hanford commented Nov 8, 2018

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

@NathanielHill
Copy link

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 pushed a commit that referenced this issue Nov 10, 2018
In the process of investigating #69, I made a [reproduction repo](https://github.com/shawninder/next-manifest-sw-clash) which would call `next build` and then read the produced files to validate stuff. Strangely, the files didn't exist yet; somehow `next.build` was resolving before the files had been written.

This PR fixes that issue. Now, the files I expect to exist as soon as next.build resolves (including main.js) always do indeed exist.

What it means for my own reproduction repo is that now the tests match the manual repro.
@hanford
Copy link
Owner Author

hanford commented Nov 11, 2018

@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
Copy link
Contributor

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
Copy link

@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? ✌️

@hanford
Copy link
Owner Author

hanford commented Nov 11, 2018

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

@hanford
Copy link
Owner Author

hanford commented Nov 11, 2018

@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
Copy link
Contributor

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
Copy link
Owner Author

hanford commented Nov 12, 2018

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 #73 b/c next-offline was no longer adding register-sw.js in the consumers code.

@shawninder

@hanford
Copy link
Owner Author

hanford commented Nov 29, 2018

@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
Copy link

@hanford, not having any problems here!

@NathanielHill
Copy link

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
Copy link
Owner Author

hanford commented Nov 29, 2018

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
Copy link

@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
Copy link
Owner Author

hanford commented Dec 3, 2018

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
Copy link

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. 👍

@hanford
Copy link
Owner Author

hanford commented Dec 21, 2018

@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
Copy link

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
Copy link

Appreciate you working on this @hanford 👌

@hanford
Copy link
Owner Author

hanford commented Dec 30, 2018

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
Copy link
Owner Author

hanford commented Dec 31, 2018

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
Copy link

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 👍

@efleurine
Copy link

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

@junibrosas
Copy link

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

@hanford hanford added bug Something isn't working help wanted Extra attention is needed labels Apr 22, 2019
@hanford hanford pinned this issue May 31, 2019
@hanford hanford unpinned this issue Jul 7, 2019
@hanford hanford pinned this issue Jul 7, 2019
@elken
Copy link

elken commented Jul 29, 2019

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
Copy link
Owner Author

hanford commented Aug 10, 2019

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

@lukovskij
Copy link

you can wrap only config object to withOffline:
withPlugins(
[[optimizedImages, {}], withBundleAnalyzer],
withOffline({
... your next.config object.
})
)

@Sawannrl123
Copy link

Sawannrl123 commented Jan 15, 2020

Hi,

I am not getting the PWA install prompt on mobile or desktop. This is my next.config.js file.

`const { PHASE_DEVELOPMENT_SERVER } = require("next/constants");
//const { withPlugins } = require("next-compose-plugins");
const withSass = require("@zeit/next-sass");
const withCSS = require("@zeit/next-css");
const withManifest = require("next-manifest");
const withOffline = require("next-offline");
const withImages = require("next-images");
const forceProd = require("./forceProd");
const manifest = require("./manifest");

const config = {
distDir: "build",
manifest: {
output: "./public/",
short_name: "New way of ordering",
name: "Zoop",
description: "New way of ordering",
dir: "ltr",
lang: "en",
icons: [
{
src: "favicon/android-icon-36x36.png",
sizes: "36x36",
type: "image/png",
density: "0.75"
},
{
src: "favicon/android-icon-48x48.png",
sizes: "48x48",
type: "image/png",
density: "1.0"
},
{
src: "favicon/android-icon-72x72.png",
sizes: "72x72",
type: "image/png",
density: "1.5"
},
{
src: "favicon/android-icon-96x96.png",
sizes: "96x96",
type: "image/png",
density: "2.0"
},
{
src: "favicon/android-icon-144x144.png",
sizes: "144x144",
type: "image/png",
density: "3.0"
},
{
src: "favicon/android-icon-192x192.png",
sizes: "192x192",
type: "image/png",
density: "4.0"
}
],
start_url: "/",
display: "standalone",
theme_color: "#FF2B22",
background_color: "#ffffff"
}
};

const hasSass = withSass(config);
const hasCss = withCSS(hasSass);
const hasImages = withImages(hasCss);
const hasManifest = withManifest(hasImages);
const hasOffline = withOffline(hasManifest);

module.exports = hasOffline;

// module.exports = withPlugins([
// [withManifest({ manifest })],
// [withOffline],
// [withImages],
// [withSass],
// [withCSS]
// ]);
`

I am checking it on an https production server.

This is my server.js file

`
const { parse } = require("url");
const { join } = require("path");

const { createServer } = require("http");
const next = require("next");
const routes = require("./routes");

const port = parseInt(process.env.PORT, 10) || 3000;
const dev = process.env.NODE_ENV !== "production";
const app = next({ dev });

const handler = routes.getRequestHandler(app, ({ req, res, route, query }) => {
const parsedUrl = parse(req.url, true);

const { pathname } = parsedUrl;

if (pathname === "/service-worker.js") {
const filePath = join(__dirname, ".next", pathname);
app.serveStatic(req, res, filePath);
} else {
app.render(req, res, route.page, query);
}
});

app.prepare().then(() => {
createServer(handler).listen(port, err => {
if (err) throw err;
console.log(> Ready on http://localhost:${port});
});
});
`

This is my routes.js file

`const nextRoutes = require("next-routes");
const routes = (module.exports = nextRoutes());

routes.add("Home", "/", "/");`

When I am checking browser console, SW is registered there. This is the SW object I got

SW registered:
Capture

Can anyone please help me out.

@AmberPoison
Copy link

AmberPoison commented Feb 12, 2020

I think it should be like this.

module.exports = withPlugins(
  [withOffline, configOffline],
  [withCSS, configWithCSS],
  ...
)

@abhijithvijayan
Copy link

For anyone who runs into the problem with next-compose-plugins, custom express server and custom service-worker script, use this.

next.config.js

const withOffline = require('next-offline');
const webpack = require('webpack');

// acts like next-compose-plugins
const pipe = (...ops) => ops.reduce((a, b) => (arg) => b(a(arg)));

const settings = {
  // Trying to set NODE_ENV=production when running yarn dev causes a build-time error so we
  // turn on the SW in dev mode so that we can actually test it
  generateInDevMode: true,
  // next-offline will not generate a service worker and will instead try to modify the file found in workboxOpts.swSrc using WorkBox's [Inject Plugin]
  generateSw: false,
  workboxOpts: {
    swSrc: './custom-sw.js', // script to be injected to the service worker bundle
    swDest: 'static/service-worker.js', // where Workbox outputs the service worker that it generates
  },

  webpack(config) {

    return config;
  },
};

// add any other plugins to this list
// https://github.com/hanford/next-offline/issues/69#issuecomment-515882561
const wrapper = pipe(withOffline);

module.exports = wrapper(settings);

server.js

...
 // serve service worker server side
  server.get('/service-worker.js', (req, res) => {
	// adjust to your next build path
    const filePath = path.join(
      __dirname,
      '.next',
      'static',
      'service-worker.js'
    );

    // Do not cache service worker
    res.set(
      'Cache-Control',
      'no-store, no-cache, must-revalidate, proxy-revalidate'
    );
    res.set('Content-Type', 'application/javascript');

    app.serveStatic(req, res, filePath);
  });
...

custom-sw.js

/* eslint-disable no-restricted-globals */
import {precacheAndRoute} from 'workbox-precaching';

precacheAndRoute(self.__WB_MANIFEST);
// Your other SW code goes here.

self.addEventListener('push', (e) => {
  // const data = e.data.json();
  const data = e.data ? JSON.parse(e.data.text()) : {};
  console.log('Push received', data);

  const options = {
    body: data.message,
    icon: 'http://localhost:4000',
    url: 'http://localhost:4001',
    vibrate: [100, 50, 100],
    data: {
      dateOfArrival: Date.now(),
      primaryKey: '2',
    },
  };

  e.waitUntil(self.registration.showNotification(data.title, options));
});

// The notification click event
// Inform when the notification close/hide from the window
self.addEventListener('notificationclick', (e) => {
  e.notification.close();
  const notificationUrl = 'http://localhost:4001';

  if (e.action === 'close') {
    e.notification.close();
  } else {
    clients.openWindow(notificationUrl);
  }
});

@SalahAdDin
Copy link

SalahAdDin commented Nov 29, 2020

I'm getting the same problem, i installed and configured this as the examples, but it does not work.

you can wrap only config object to withOffline:
withPlugins(
[[optimizedImages, {}], withBundleAnalyzer],
withOffline({
... your next.config object.
})
)

It does work in this way.

I think it should be like this.

module.exports = withPlugins(
  [withOffline, configOffline],
  [withCSS, configWithCSS],
  ...
)

This does not work in this way even when it should do.

@hanford Is there any way to do this with the next-compose-plugins way?

oknoorap added a commit to oknoorap/tvlix that referenced this issue Mar 31, 2021
mccoyplayer pushed a commit to mccoyplayer/NEXT that referenced this issue Feb 9, 2024
In the process of investigating hanford/next-offline#69, I made a [reproduction repo](https://github.com/shawninder/next-manifest-sw-clash) which would call `next build` and then read the produced files to validate stuff. Strangely, the files didn't exist yet; somehow `next.build` was resolving before the files had been written.

This PR fixes that issue. Now, the files I expect to exist as soon as next.build resolves (including main.js) always do indeed exist.

What it means for my own reproduction repo is that now the tests match the manual repro.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests