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

[feature] add option to disable converting ISO strings to dates #635

Merged
merged 3 commits into from
Oct 25, 2022

Conversation

chrisradek
Copy link
Contributor

This adds a new load option - disableAutoISOConversions. This turns off the automatic conversion of event fields where any strings that appear as an ISO8601 string (including just dates, e.g. YYYY-MM-DD) become Date objects.

Background

Through @segment/facade, any events passed to classic integrations had any ISO strings converted to Dates. I suspect this was intentional behavior because the way we cloned events (JSON.parse(JSON.stringify(event))) converted Dates to ISO8601 strings, so facade was converting them back to dates.

However, some users want their ISO strings to be passed to integrations as is, especially when the string represents a calendar date and not a time.

PR details

@segment/facade supports turning off this conversion by setting traverse to false, so the new option ultimately sets the traverse flag.

I also changed how we clone events so that users can still specify a property as a Date object and have that carried over to the cloned event regardless of the new setting. It looks like analytics.js classic also did a deep clone of the event instead of our JSON.parse(JSON.stringify(event)) trick, so this shouldn't cause any unintended consequences.

I made this field default to false, meaning users need to opt-in to this new behavior. I made it opt-in to prevent potentially impacting user-defined destination middleware.

[x] I've included a changeset (psst. run yarn changeset. Read about changesets here).

@changeset-bot
Copy link

changeset-bot bot commented Oct 21, 2022

🦋 Changeset detected

Latest commit: 88cb9c2

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-next Minor

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

@silesky
Copy link
Contributor

silesky commented Oct 25, 2022

Very happy to get rid of JSON.parse(JSON.stringify(event))! That's a pretty worrying / destructive operation in general.

@chrisradek chrisradek merged commit 222d4ec into master Oct 25, 2022
@chrisradek chrisradek deleted the disable-traversal branch October 25, 2022 20:53
@github-actions github-actions bot mentioned this pull request Oct 25, 2022
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.

2 participants