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

Made remote "notify" commands work #1467

Closed
wants to merge 1 commit into from
Closed

Conversation

mogsie
Copy link

@mogsie mogsie commented Dec 18, 2017

The notifier command fails with a message that browserSync.events is undefined; this is because the commands receive the emitter as the parameter, not the browserSync instance.

At least this change makes it possible to have tools run things like

curl 'http://localhost:3000/__browser_sync__?method=notify&args=Yeah<br><em>cool</em>!!'

:)

The notifier command fails with a message that browserSync.events is undefined; this is because the commands receive the emitter as the parameter, not the browserSync instance.

At least this change makes it possible to have tools run things like

    curl 'http://localhost:3000/__browser_sync__?method=notify&args=Yeah<br><em>cool</em>!!'

:)
@shakyShane
Copy link
Contributor

@mogsie Thanks for spotting this!

I couldn't apply your fix as there are too many places relying on the original implementation.

It was a mistake to sometimes pass the emitter, and sometimes the BS instance, but we'll have to wait for a major sem ver to fix

browser-sync/index.js

Lines 308 to 313 in 0aa00a7

exit: require("./lib/public/exit")(browserSync),
notify: require("./lib/public/notify")(browserSync),
pause: require("./lib/public/pause")(browserSync),
resume: require("./lib/public/resume")(browserSync),
reload: require("./lib/public/reload")(emitter),
stream: require("./lib/public/stream")(emitter),

@mogsie
Copy link
Author

mogsie commented Dec 25, 2017

Sure thing :) Maybe a new public API method could be put in place in order for notifications to work in the interim?

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

2 participants