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

Allow notifyOptions to depend on compilation status #70

Merged
merged 2 commits into from Jun 19, 2021

Conversation

astorije
Copy link
Contributor

This allows for the following scenario: on success, clicking the notification should open the development URL (when running the dev server); on error, clicking the notification should open the terminal window. This is only possible if evaluating notifyOptions dynamically when calling notifier.notify instead of when the plugin is instantiated.

// webpack.config.ts
import WebpackBuildNotifierPlugin from 'webpack-build-notifier';

let address: string;

export default {
  // ... snip ...
  devServer: {
    // ... snip ...
    onListening(server) {
      const { port } = server.listeningApp.address();
      address = `http://localhost:${port}`;
    },
  },
  plugins: [
    // ... snip ...
    new WebpackBuildNotifierPlugin({
      notifyOptions(success) {
        return address && status === 'success' ? { open: address } : undefined;
      },
    }),
  ],
  // ... snip ...
};

This is particularly useful combined with #67 as it allows us to display Click here to open <address> on success only and use the default error/warning message otherwise. And combined with #68, it allows for nicely readable error/warning messages as well :)

@astorije
Copy link
Contributor Author

astorije commented Apr 26, 2021

I'm not sure why all these builds are failing. Travis status and Coveralls status are both 🟢 green, yet these return 500s.
The other PRs were green yesterday, and when I rebased later on it started failing. It does seem like it's coming from one of the 2 vendors.

EDIT: Trying to open the Coveralls build on the latest master commit (which was green as well), there does seem to be a 500... https://coveralls.io/builds/37873324

EDIT2: Alright then lemurheavy/coveralls-public#1543

EDIT3: Fixed!

@astorije astorije force-pushed the astorije/notify-options-callback branch from 9a07359 to 1bb9f85 Compare April 26, 2021 16:16
@coveralls
Copy link

coveralls commented Apr 26, 2021

Pull Request Test Coverage Report for Build 162

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 93.778%

Totals Coverage Status
Change from base Build 160: 0.1%
Covered Lines: 131
Relevant Lines: 135

💛 - Coveralls

@astorije astorije force-pushed the astorije/notify-options-callback branch from 1bb9f85 to 6e709e0 Compare April 28, 2021 14:28
@astorije
Copy link
Contributor Author

@RoccoC, rebased on master and fixed conflicts!

@RoccoC
Copy link
Owner

RoccoC commented Apr 28, 2021

Great, I will review this one shortly, thanks again!

@astorije astorije force-pushed the astorije/notify-options-callback branch from 6e709e0 to 4bc839a Compare May 3, 2021 00:10
@astorije
Copy link
Contributor Author

astorije commented May 3, 2021

@RoccoC, I rebased/fixed conflicts after your recent type consolidation changes, let me know if there is anything I can do to help, I'd love to be able to use this! :)

@astorije
Copy link
Contributor Author

Hey @RoccoC, did you get a chance to look at this? :) Happy to help this project further!

@RoccoC
Copy link
Owner

RoccoC commented Jun 18, 2021

Hey @astorije , thanks for the reminder -- and sorry, no, I haven't re-reviewed this yet, but will do so today! Thanks again for your contributions!

@astorije
Copy link
Contributor Author

My pleasure and thank you, happy to help! :)

@RoccoC RoccoC merged commit 1e20343 into RoccoC:master Jun 19, 2021
@RoccoC
Copy link
Owner

RoccoC commented Jun 19, 2021

Merged and published in 2.3.0

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.

None yet

3 participants