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

Service Worker required when disabled in dev #1594

Closed
tysonmatanich opened this issue Sep 15, 2021 · 7 comments · Fixed by #1595
Closed

Service Worker required when disabled in dev #1594

tysonmatanich opened this issue Sep 15, 2021 · 7 comments · Fixed by #1595

Comments

@tysonmatanich
Copy link

What is the current behaviour?
When running preact watch --sw false it still tries to attach a service worker. preact watch works fine which should default sw to false (https://github.com/preactjs/preact-cli#preact-watch).

Steps to Reproduce

  1. Use a project without a .\src\sw.js
  2. Run preact watch --sw false
  3. See error

Build failed!

× ERROR Entry module not found: Error: Can't resolve '.\src\sw.js' in '.\src'
× ERROR InjectManifest: Entry module not found: Error: Can't resolve '.\src\sw.js' in '.\src'

What is the expected behaviour?
Build should succeed using same behavior as preact watch which defaults sw to false

Please mention any other relevant information
I'm assuming it was introduced in 9f9277b

Environment Info:

System:
OS: Windows 10 10.0.19043
Binaries:
Node: 14.16.1 - C:\Program Files\nodejs\node.EXE
npm: 6.14.12 - C:\Program Files\nodejs\npm.CMD
Browsers:
Edge: Spartan (44.19041.1023.0), Chromium (93.0.961.47)
npmPackages:
preact: ^10.5.14 => 10.5.14
preact-cli: ^3.2.2 => 3.2.2
preact-render-to-string: ^5.1.19 => 5.1.19
preact-router: ^3.2.1 => 3.2.1

@developit
Copy link
Member

any chance this works correctly via preact watch --no-sw?

@tysonmatanich
Copy link
Author

Yes it does

preact watch
preact watch --no-sw
preact watch --sw false

@rschristian
Copy link
Member

Looks like we're not converting the "false" into a boolean in watch mode as we do in build, just passing the string value through.

Will try to get a fix together here.

@tysonmatanich
Copy link
Author

It also seems the changes in 9f9277b broke the workaround in #955 (comment) for using a ServiceWorker in watch mode.

@rschristian
Copy link
Member

@tysonmatanich What doesn't seem to be working? That workaround, as far as I can tell, just matches what's already provided.

That exact config should be broken, as using it will result in duplicate plugins being pushed.

@tysonmatanich
Copy link
Author

The workaround allows for the custom serviceworker to be used when running with preact watch --sw, hence the if(env.isWatch)

@rschristian
Copy link
Member

Sorry, I don't understand. You can customize the config as-is:

export default {
    /**
     * Function that mutates the original webpack config.
     * Supports asynchronous changes when a promise is returned (or it's an async function).
     *
     * @param {object} config - original webpack config.
     * @param {object} env - options passed to the CLI.
     * @param {WebpackConfigHelpers} helpers - object with useful helpers for working with the webpack config.
     **/
    webpack(config, env, helpers) {
        if (env.isWatch) {
            const manifestPlugin = helpers.getPluginsByName(config, 'InjectManifest')[0];
            // You can make any modifications you'd like to the manifestPlugin here
        }    
    }
};

bors bot pushed a commit that referenced this issue Sep 30, 2021
**What kind of change does this PR introduce?**

Bugfix

**Did you add tests for your changes?**

No

**Summary**

Fixes #1594 

`preact watch --sw false` passed the value through as a string, rather than coercing to a boolean, which was an oversight.

**Does this PR introduce a breaking change?**

No
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 a pull request may close this issue.

3 participants