Add enable option to disable flushing

[vadimdemedes]

Fixes#116.

Allows to silently skip sending of messages, which is useful in tests and in development. Example usage:

constclient=newClient('write key',{enable: process.env.NODE_ENV==='production'})

[stephenmathieson]

Duplicate (kinda) of #90 / #75. I'm still unconvinced the shim (see that issue) is worth the added maintenance here.

[hulbert]

We're about to add this lib to our systems, and this would be a very nice thing to have. It seems the proposed method of overriding the flush method relies on undocumented behavior that could break between versions.

[vadimdemedes]

@hulbert is absolutely on point. This is something I'd want too. Recommending overriding flush() method is a hackish workaround. I think this option clearly has a practical use case and we need to support one way or another.

Alternatively we could look at NODE_ENV and flush only when it's equal to "production". This is what Bugsnag's Node.js library does and I found it a nice convention. Similar to Bugsnag, we would assume environment is production by default, if NODE_ENV is empty. If there's a custom way of setting the environment (not setting NODE_ENV), we'd have env option to override the env name:

newClient('write key',{env: 'production'})

Even though it's good, I still prefer having enable, because it doesn't make any assumptions about the environment and is explicit. It gives complete control to the user and strips us from maintenance issues with the env way.

[vadimdemedes]

@stephenmathieson what do you think?

[stephenmathieson]

we could look at NODE_ENV and flush only when it's equal to "production".

While this does seem ideal in many cases, it would break our analytics in our non-prod environments. For example, on .build, we set NODE_ENV=staging, but we still want to send analytics calls (for testing their implementation, etc).

we'd have env option to override the env name

I'm 👎 here. Setting env = "whatever" seems magical in this case. I'd much prefer something explicit.

I still prefer having enable, because it doesn't make any assumptions about the environment and is explicit.

Agreed.

what do you think?

Since there is an obvious demand here, I think this can land. If you fix up the conflicts, I'll approve/merge. 😄

[vadimdemedes]

I agree on your argument against env option, @stephenmathieson. Explicit option is better. PR is fixed up and ready to get merged!

[stephenmathieson]

Strange use of a ternary here (seems the linter agrees with me). Any reason this.enabled = options.enable won't work?

[vadimdemedes]

No idea what happened here 😆

[stephenmathieson]

One small thing, but this LGTM!

[f2prateek]

What if a customer does this:

const client = new Client('write key', {
enable: process.env.NODE_ENV === 'production'
})
client.track(...)
client.enabled = false
client.flush() // either explicitly or as part of a timer 

Basically a customer might want to track some events, and then disable the client but still have the expectation of flushing the previously queued events.

I'd prefer if the enabled flag was not mutable. I think the JS convention is to prefix it with _?

[stephenmathieson]

@f2prateek we could just make the enabled field immutable so that an error would be raised instead

[f2prateek]

SGTM!

[stephenmathieson]

@vadimdemedes think you could make the enabled field immutable? Something like this should do:

Object.defineProperty(this,'enabled',{configurable: false,writable: false,value: enabled})

[f2prateek]

Closing this for now, feel free to re-open once the PR is updated - otherwise the libraries team will tackle it soon. Tracking here https://segment.atlassian.net/browse/LIB-118

[vadimdemedes]

Fixes#116.

Allows to silently skip sending of messages, which is useful in tests and in development. Example usage:

constclient=newClient('write key',{enable: process.env.NODE_ENV==='production'})

[stephenmathieson]

Duplicate (kinda) of #90 / #75. I'm still unconvinced the shim (see that issue) is worth the added maintenance here.

[hulbert]

We're about to add this lib to our systems, and this would be a very nice thing to have. It seems the proposed method of overriding the flush method relies on undocumented behavior that could break between versions.

[vadimdemedes]

@hulbert is absolutely on point. This is something I'd want too. Recommending overriding flush() method is a hackish workaround. I think this option clearly has a practical use case and we need to support one way or another.

Alternatively we could look at NODE_ENV and flush only when it's equal to "production". This is what Bugsnag's Node.js library does and I found it a nice convention. Similar to Bugsnag, we would assume environment is production by default, if NODE_ENV is empty. If there's a custom way of setting the environment (not setting NODE_ENV), we'd have env option to override the env name:

newClient('write key',{env: 'production'})

Even though it's good, I still prefer having enable, because it doesn't make any assumptions about the environment and is explicit. It gives complete control to the user and strips us from maintenance issues with the env way.

[vadimdemedes]

@stephenmathieson what do you think?

[stephenmathieson]

we could look at NODE_ENV and flush only when it's equal to "production".

While this does seem ideal in many cases, it would break our analytics in our non-prod environments. For example, on .build, we set NODE_ENV=staging, but we still want to send analytics calls (for testing their implementation, etc).

we'd have env option to override the env name

I'm 👎 here. Setting env = "whatever" seems magical in this case. I'd much prefer something explicit.

I still prefer having enable, because it doesn't make any assumptions about the environment and is explicit.

Agreed.

what do you think?

Since there is an obvious demand here, I think this can land. If you fix up the conflicts, I'll approve/merge. 😄

[vadimdemedes]

I agree on your argument against env option, @stephenmathieson. Explicit option is better. PR is fixed up and ready to get merged!

[stephenmathieson]

Strange use of a ternary here (seems the linter agrees with me). Any reason this.enabled = options.enable won't work?

[vadimdemedes]

No idea what happened here 😆

[stephenmathieson]

One small thing, but this LGTM!

[f2prateek]

What if a customer does this:

const client = new Client('write key', {
enable: process.env.NODE_ENV === 'production'
})
client.track(...)
client.enabled = false
client.flush() // either explicitly or as part of a timer 

Basically a customer might want to track some events, and then disable the client but still have the expectation of flushing the previously queued events.

I'd prefer if the enabled flag was not mutable. I think the JS convention is to prefix it with _?

[stephenmathieson]

@f2prateek we could just make the enabled field immutable so that an error would be raised instead

[f2prateek]

SGTM!

[stephenmathieson]

@vadimdemedes think you could make the enabled field immutable? Something like this should do:

Object.defineProperty(this,'enabled',{configurable: false,writable: false,value: enabled})

[f2prateek]

Closing this for now, feel free to re-open once the PR is updated - otherwise the libraries team will tackle it soon. Tracking here https://segment.atlassian.net/browse/LIB-118

[vadimdemedes]

Fixes#116.

Allows to silently skip sending of messages, which is useful in tests and in development. Example usage:

constclient=newClient('write key',{enable: process.env.NODE_ENV==='production'})

[stephenmathieson]

Duplicate (kinda) of #90 / #75. I'm still unconvinced the shim (see that issue) is worth the added maintenance here.

[hulbert]

We're about to add this lib to our systems, and this would be a very nice thing to have. It seems the proposed method of overriding the flush method relies on undocumented behavior that could break between versions.

[vadimdemedes]

@hulbert is absolutely on point. This is something I'd want too. Recommending overriding flush() method is a hackish workaround. I think this option clearly has a practical use case and we need to support one way or another.

Alternatively we could look at NODE_ENV and flush only when it's equal to "production". This is what Bugsnag's Node.js library does and I found it a nice convention. Similar to Bugsnag, we would assume environment is production by default, if NODE_ENV is empty. If there's a custom way of setting the environment (not setting NODE_ENV), we'd have env option to override the env name:

newClient('write key',{env: 'production'})

Even though it's good, I still prefer having enable, because it doesn't make any assumptions about the environment and is explicit. It gives complete control to the user and strips us from maintenance issues with the env way.

[vadimdemedes]

@stephenmathieson what do you think?

[stephenmathieson]

we could look at NODE_ENV and flush only when it's equal to "production".

While this does seem ideal in many cases, it would break our analytics in our non-prod environments. For example, on .build, we set NODE_ENV=staging, but we still want to send analytics calls (for testing their implementation, etc).

we'd have env option to override the env name

I'm 👎 here. Setting env = "whatever" seems magical in this case. I'd much prefer something explicit.

I still prefer having enable, because it doesn't make any assumptions about the environment and is explicit.

Agreed.

what do you think?

Since there is an obvious demand here, I think this can land. If you fix up the conflicts, I'll approve/merge. 😄

[vadimdemedes]

I agree on your argument against env option, @stephenmathieson. Explicit option is better. PR is fixed up and ready to get merged!

[stephenmathieson]

Strange use of a ternary here (seems the linter agrees with me). Any reason this.enabled = options.enable won't work?

[vadimdemedes]

No idea what happened here 😆

[stephenmathieson]

One small thing, but this LGTM!

[f2prateek]

What if a customer does this:

const client = new Client('write key', {
enable: process.env.NODE_ENV === 'production'
})
client.track(...)
client.enabled = false
client.flush() // either explicitly or as part of a timer 

Basically a customer might want to track some events, and then disable the client but still have the expectation of flushing the previously queued events.

I'd prefer if the enabled flag was not mutable. I think the JS convention is to prefix it with _?

[stephenmathieson]

@f2prateek we could just make the enabled field immutable so that an error would be raised instead

[f2prateek]

SGTM!

[stephenmathieson]

@vadimdemedes think you could make the enabled field immutable? Something like this should do:

Object.defineProperty(this,'enabled',{configurable: false,writable: false,value: enabled})

[f2prateek]

Closing this for now, feel free to re-open once the PR is updated - otherwise the libraries team will tackle it soon. Tracking here https://segment.atlassian.net/browse/LIB-118

[vadimdemedes]

Fixes#116.

Allows to silently skip sending of messages, which is useful in tests and in development. Example usage:

constclient=newClient('write key',{enable: process.env.NODE_ENV==='production'})

[stephenmathieson]

Duplicate (kinda) of #90 / #75. I'm still unconvinced the shim (see that issue) is worth the added maintenance here.

[hulbert]

We're about to add this lib to our systems, and this would be a very nice thing to have. It seems the proposed method of overriding the flush method relies on undocumented behavior that could break between versions.

[vadimdemedes]

@hulbert is absolutely on point. This is something I'd want too. Recommending overriding flush() method is a hackish workaround. I think this option clearly has a practical use case and we need to support one way or another.

Alternatively we could look at NODE_ENV and flush only when it's equal to "production". This is what Bugsnag's Node.js library does and I found it a nice convention. Similar to Bugsnag, we would assume environment is production by default, if NODE_ENV is empty. If there's a custom way of setting the environment (not setting NODE_ENV), we'd have env option to override the env name:

newClient('write key',{env: 'production'})

Even though it's good, I still prefer having enable, because it doesn't make any assumptions about the environment and is explicit. It gives complete control to the user and strips us from maintenance issues with the env way.

[vadimdemedes]

@stephenmathieson what do you think?

[stephenmathieson]

we could look at NODE_ENV and flush only when it's equal to "production".

While this does seem ideal in many cases, it would break our analytics in our non-prod environments. For example, on .build, we set NODE_ENV=staging, but we still want to send analytics calls (for testing their implementation, etc).

we'd have env option to override the env name

I'm 👎 here. Setting env = "whatever" seems magical in this case. I'd much prefer something explicit.

I still prefer having enable, because it doesn't make any assumptions about the environment and is explicit.

Agreed.

what do you think?

Since there is an obvious demand here, I think this can land. If you fix up the conflicts, I'll approve/merge. 😄

[vadimdemedes]

I agree on your argument against env option, @stephenmathieson. Explicit option is better. PR is fixed up and ready to get merged!

[stephenmathieson]

Strange use of a ternary here (seems the linter agrees with me). Any reason this.enabled = options.enable won't work?

[vadimdemedes]

No idea what happened here 😆

[stephenmathieson]

One small thing, but this LGTM!

[f2prateek]

What if a customer does this:

const client = new Client('write key', {
enable: process.env.NODE_ENV === 'production'
})
client.track(...)
client.enabled = false
client.flush() // either explicitly or as part of a timer 

Basically a customer might want to track some events, and then disable the client but still have the expectation of flushing the previously queued events.

I'd prefer if the enabled flag was not mutable. I think the JS convention is to prefix it with _?

[stephenmathieson]

@f2prateek we could just make the enabled field immutable so that an error would be raised instead

[f2prateek]

SGTM!

[stephenmathieson]

@vadimdemedes think you could make the enabled field immutable? Something like this should do:

Object.defineProperty(this,'enabled',{configurable: false,writable: false,value: enabled})

[f2prateek]

Closing this for now, feel free to re-open once the PR is updated - otherwise the libraries team will tackle it soon. Tracking here https://segment.atlassian.net/browse/LIB-118

[vadimdemedes]

Fixes#116.

Allows to silently skip sending of messages, which is useful in tests and in development. Example usage:

constclient=newClient('write key',{enable: process.env.NODE_ENV==='production'})

[stephenmathieson]

Duplicate (kinda) of #90 / #75. I'm still unconvinced the shim (see that issue) is worth the added maintenance here.

[hulbert]

We're about to add this lib to our systems, and this would be a very nice thing to have. It seems the proposed method of overriding the flush method relies on undocumented behavior that could break between versions.

[vadimdemedes]

@hulbert is absolutely on point. This is something I'd want too. Recommending overriding flush() method is a hackish workaround. I think this option clearly has a practical use case and we need to support one way or another.

Alternatively we could look at NODE_ENV and flush only when it's equal to "production". This is what Bugsnag's Node.js library does and I found it a nice convention. Similar to Bugsnag, we would assume environment is production by default, if NODE_ENV is empty. If there's a custom way of setting the environment (not setting NODE_ENV), we'd have env option to override the env name:

newClient('write key',{env: 'production'})

Even though it's good, I still prefer having enable, because it doesn't make any assumptions about the environment and is explicit. It gives complete control to the user and strips us from maintenance issues with the env way.

[vadimdemedes]

@stephenmathieson what do you think?

[stephenmathieson]

we could look at NODE_ENV and flush only when it's equal to "production".

While this does seem ideal in many cases, it would break our analytics in our non-prod environments. For example, on .build, we set NODE_ENV=staging, but we still want to send analytics calls (for testing their implementation, etc).

we'd have env option to override the env name

I'm 👎 here. Setting env = "whatever" seems magical in this case. I'd much prefer something explicit.

I still prefer having enable, because it doesn't make any assumptions about the environment and is explicit.

Agreed.

what do you think?

Since there is an obvious demand here, I think this can land. If you fix up the conflicts, I'll approve/merge. 😄

[vadimdemedes]

I agree on your argument against env option, @stephenmathieson. Explicit option is better. PR is fixed up and ready to get merged!

[stephenmathieson]

Strange use of a ternary here (seems the linter agrees with me). Any reason this.enabled = options.enable won't work?

[vadimdemedes]

No idea what happened here 😆

[stephenmathieson]

One small thing, but this LGTM!

[f2prateek]

What if a customer does this:

const client = new Client('write key', {
enable: process.env.NODE_ENV === 'production'
})
client.track(...)
client.enabled = false
client.flush() // either explicitly or as part of a timer 

Basically a customer might want to track some events, and then disable the client but still have the expectation of flushing the previously queued events.

I'd prefer if the enabled flag was not mutable. I think the JS convention is to prefix it with _?

[stephenmathieson]

@f2prateek we could just make the enabled field immutable so that an error would be raised instead

[f2prateek]

SGTM!

[stephenmathieson]

@vadimdemedes think you could make the enabled field immutable? Something like this should do:

Object.defineProperty(this,'enabled',{configurable: false,writable: false,value: enabled})

[f2prateek]

Closing this for now, feel free to re-open once the PR is updated - otherwise the libraries team will tackle it soon. Tracking here https://segment.atlassian.net/browse/LIB-118

[vadimdemedes]

Fixes#116.

Allows to silently skip sending of messages, which is useful in tests and in development. Example usage:

constclient=newClient('write key',{enable: process.env.NODE_ENV==='production'})

[stephenmathieson]

Duplicate (kinda) of #90 / #75. I'm still unconvinced the shim (see that issue) is worth the added maintenance here.

[hulbert]

We're about to add this lib to our systems, and this would be a very nice thing to have. It seems the proposed method of overriding the flush method relies on undocumented behavior that could break between versions.

[vadimdemedes]

@hulbert is absolutely on point. This is something I'd want too. Recommending overriding flush() method is a hackish workaround. I think this option clearly has a practical use case and we need to support one way or another.

Alternatively we could look at NODE_ENV and flush only when it's equal to "production". This is what Bugsnag's Node.js library does and I found it a nice convention. Similar to Bugsnag, we would assume environment is production by default, if NODE_ENV is empty. If there's a custom way of setting the environment (not setting NODE_ENV), we'd have env option to override the env name:

newClient('write key',{env: 'production'})

Even though it's good, I still prefer having enable, because it doesn't make any assumptions about the environment and is explicit. It gives complete control to the user and strips us from maintenance issues with the env way.

[vadimdemedes]

@stephenmathieson what do you think?

[stephenmathieson]

we could look at NODE_ENV and flush only when it's equal to "production".

While this does seem ideal in many cases, it would break our analytics in our non-prod environments. For example, on .build, we set NODE_ENV=staging, but we still want to send analytics calls (for testing their implementation, etc).

we'd have env option to override the env name

I'm 👎 here. Setting env = "whatever" seems magical in this case. I'd much prefer something explicit.

I still prefer having enable, because it doesn't make any assumptions about the environment and is explicit.

Agreed.

what do you think?

Since there is an obvious demand here, I think this can land. If you fix up the conflicts, I'll approve/merge. 😄

[vadimdemedes]

I agree on your argument against env option, @stephenmathieson. Explicit option is better. PR is fixed up and ready to get merged!

[stephenmathieson]

Strange use of a ternary here (seems the linter agrees with me). Any reason this.enabled = options.enable won't work?

[vadimdemedes]

No idea what happened here 😆

[stephenmathieson]

One small thing, but this LGTM!

[f2prateek]

What if a customer does this:

const client = new Client('write key', {
enable: process.env.NODE_ENV === 'production'
})
client.track(...)
client.enabled = false
client.flush() // either explicitly or as part of a timer 

Basically a customer might want to track some events, and then disable the client but still have the expectation of flushing the previously queued events.

I'd prefer if the enabled flag was not mutable. I think the JS convention is to prefix it with _?

[stephenmathieson]

@f2prateek we could just make the enabled field immutable so that an error would be raised instead

[f2prateek]

SGTM!

[stephenmathieson]

@vadimdemedes think you could make the enabled field immutable? Something like this should do:

Object.defineProperty(this,'enabled',{configurable: false,writable: false,value: enabled})

[f2prateek]

Closing this for now, feel free to re-open once the PR is updated - otherwise the libraries team will tackle it soon. Tracking here https://segment.atlassian.net/browse/LIB-118

[vadimdemedes]

Fixes#116.

Allows to silently skip sending of messages, which is useful in tests and in development. Example usage:

constclient=newClient('write key',{enable: process.env.NODE_ENV==='production'})

[stephenmathieson]

Duplicate (kinda) of #90 / #75. I'm still unconvinced the shim (see that issue) is worth the added maintenance here.

[hulbert]

We're about to add this lib to our systems, and this would be a very nice thing to have. It seems the proposed method of overriding the flush method relies on undocumented behavior that could break between versions.

[vadimdemedes]

@hulbert is absolutely on point. This is something I'd want too. Recommending overriding flush() method is a hackish workaround. I think this option clearly has a practical use case and we need to support one way or another.

Alternatively we could look at NODE_ENV and flush only when it's equal to "production". This is what Bugsnag's Node.js library does and I found it a nice convention. Similar to Bugsnag, we would assume environment is production by default, if NODE_ENV is empty. If there's a custom way of setting the environment (not setting NODE_ENV), we'd have env option to override the env name:

newClient('write key',{env: 'production'})

Even though it's good, I still prefer having enable, because it doesn't make any assumptions about the environment and is explicit. It gives complete control to the user and strips us from maintenance issues with the env way.

[vadimdemedes]

@stephenmathieson what do you think?

[stephenmathieson]

we could look at NODE_ENV and flush only when it's equal to "production".

While this does seem ideal in many cases, it would break our analytics in our non-prod environments. For example, on .build, we set NODE_ENV=staging, but we still want to send analytics calls (for testing their implementation, etc).

we'd have env option to override the env name

I'm 👎 here. Setting env = "whatever" seems magical in this case. I'd much prefer something explicit.

I still prefer having enable, because it doesn't make any assumptions about the environment and is explicit.

Agreed.

what do you think?

Since there is an obvious demand here, I think this can land. If you fix up the conflicts, I'll approve/merge. 😄

[vadimdemedes]

I agree on your argument against env option, @stephenmathieson. Explicit option is better. PR is fixed up and ready to get merged!

[stephenmathieson]

Strange use of a ternary here (seems the linter agrees with me). Any reason this.enabled = options.enable won't work?

[vadimdemedes]

No idea what happened here 😆

[stephenmathieson]

One small thing, but this LGTM!

[f2prateek]

What if a customer does this:

const client = new Client('write key', {
enable: process.env.NODE_ENV === 'production'
})
client.track(...)
client.enabled = false
client.flush() // either explicitly or as part of a timer 

Basically a customer might want to track some events, and then disable the client but still have the expectation of flushing the previously queued events.

I'd prefer if the enabled flag was not mutable. I think the JS convention is to prefix it with _?

[stephenmathieson]

@f2prateek we could just make the enabled field immutable so that an error would be raised instead

[f2prateek]

SGTM!

[stephenmathieson]

@vadimdemedes think you could make the enabled field immutable? Something like this should do:

Object.defineProperty(this,'enabled',{configurable: false,writable: false,value: enabled})

[f2prateek]

Closing this for now, feel free to re-open once the PR is updated - otherwise the libraries team will tackle it soon. Tracking here https://segment.atlassian.net/browse/LIB-118

[vadimdemedes]

Fixes#116.

Allows to silently skip sending of messages, which is useful in tests and in development. Example usage:

constclient=newClient('write key',{enable: process.env.NODE_ENV==='production'})

[stephenmathieson]

Duplicate (kinda) of #90 / #75. I'm still unconvinced the shim (see that issue) is worth the added maintenance here.

[hulbert]

We're about to add this lib to our systems, and this would be a very nice thing to have. It seems the proposed method of overriding the flush method relies on undocumented behavior that could break between versions.

[vadimdemedes]

@hulbert is absolutely on point. This is something I'd want too. Recommending overriding flush() method is a hackish workaround. I think this option clearly has a practical use case and we need to support one way or another.

Alternatively we could look at NODE_ENV and flush only when it's equal to "production". This is what Bugsnag's Node.js library does and I found it a nice convention. Similar to Bugsnag, we would assume environment is production by default, if NODE_ENV is empty. If there's a custom way of setting the environment (not setting NODE_ENV), we'd have env option to override the env name:

newClient('write key',{env: 'production'})

Even though it's good, I still prefer having enable, because it doesn't make any assumptions about the environment and is explicit. It gives complete control to the user and strips us from maintenance issues with the env way.

[vadimdemedes]

@stephenmathieson what do you think?

[stephenmathieson]

we could look at NODE_ENV and flush only when it's equal to "production".

While this does seem ideal in many cases, it would break our analytics in our non-prod environments. For example, on .build, we set NODE_ENV=staging, but we still want to send analytics calls (for testing their implementation, etc).

we'd have env option to override the env name

I'm 👎 here. Setting env = "whatever" seems magical in this case. I'd much prefer something explicit.

I still prefer having enable, because it doesn't make any assumptions about the environment and is explicit.

Agreed.

what do you think?

Since there is an obvious demand here, I think this can land. If you fix up the conflicts, I'll approve/merge. 😄

[vadimdemedes]

I agree on your argument against env option, @stephenmathieson. Explicit option is better. PR is fixed up and ready to get merged!

[stephenmathieson]

Strange use of a ternary here (seems the linter agrees with me). Any reason this.enabled = options.enable won't work?

[vadimdemedes]

No idea what happened here 😆

[stephenmathieson]

One small thing, but this LGTM!

[f2prateek]

What if a customer does this:

const client = new Client('write key', {
enable: process.env.NODE_ENV === 'production'
})
client.track(...)
client.enabled = false
client.flush() // either explicitly or as part of a timer 

Basically a customer might want to track some events, and then disable the client but still have the expectation of flushing the previously queued events.

I'd prefer if the enabled flag was not mutable. I think the JS convention is to prefix it with _?

[stephenmathieson]

@f2prateek we could just make the enabled field immutable so that an error would be raised instead

[f2prateek]

SGTM!

[stephenmathieson]

@vadimdemedes think you could make the enabled field immutable? Something like this should do:

Object.defineProperty(this,'enabled',{configurable: false,writable: false,value: enabled})

[f2prateek]

Closing this for now, feel free to re-open once the PR is updated - otherwise the libraries team will tackle it soon. Tracking here https://segment.atlassian.net/browse/LIB-118