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

Do not throw error when analytics are disabled. #901

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Pruxis
Copy link

@Pruxis Pruxis commented Jul 15, 2023

Imagine the following case where you disable segment in your code.

const { Analytics } = require('@segment/analytics-node');
new Analytics({ writeKey: processe.env.WRITE_KEY || '', disable: true });

If WRITE_KEY is not present, but your analytics are disabled this still throws an error.

This would simplify local setups and do not force them to have a truthy string as write key

Imagine the following case where you disable segment in your code.

const { Analytics } = require('@segment/analytics-node');
new Analytics({ writeKey: processe.env.WRITE_KEY || '', disable: true });

If WRITE_KEY is not present, but your analytics are disabled this still throws an error.

This would simplify local setups and do not force them to have a truthy string as write key
@changeset-bot
Copy link

changeset-bot bot commented Jul 15, 2023

🦋 Changeset detected

Latest commit: d25217d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@segment/analytics-node Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@chrisradek
Copy link
Contributor

@Pruxis Thanks for opening this pull request! I think from a TypeScript perspective writeKey will still be required with this change - @silesky do you think we should update types as well (I think we'd need a tagged union to make this nice) or really just keep this a runtime check?

@silesky
Copy link
Contributor

silesky commented Jul 18, 2023

@Pruxis Thanks for opening this pull request! I think from a TypeScript perspective writeKey will still be required with this change - @silesky do you think we should update types as well (I think we'd need a tagged union to make this nice) or really just keep this a runtime check?

Runtime only; no Typescript changes are needed here IMO.

@Pruxis
Copy link
Author

Pruxis commented Aug 11, 2023

@silesky Do I need to write some tests to get this merged?

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