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

fix(project config): Don't mutate the datafile object #462

Merged
merged 3 commits into from
Apr 22, 2020

Conversation

mjc1283
Copy link
Contributor

@mjc1283 mjc1283 commented Apr 21, 2020

Summary

createProjectConfig was destructively mutating its datafile argument. After that mutation, the datafile object could not be re-used to create another instance.
With this change, createProjectConfig copies the objects it needs to mutate and mutates only those copies. It's not a full deep clone; it only shallowly clones audiences, experiments, feature flags, groups, rollouts, group experiments, and rollout experiments.

Test plan

  • Updated & new unit tests
  • Compatibility suite should pass

Issues

https://optimizely.atlassian.net/browse/OASIS-6301

@mjc1283 mjc1283 added the WIP label Apr 21, 2020
@mjc1283 mjc1283 requested a review from a team as a code owner April 21, 2020 21:55
@mjc1283 mjc1283 self-assigned this Apr 21, 2020
Copy link
Contributor

@mikeproeng37 mikeproeng37 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could end up needing to be updated as we change datafile schema in the future. Would it make sense to roll our own cloneDeep function using recursion like it's done here: https://gist.githubusercontent.com/djD-REK/e8b1497e7fbf0374e4eada669e5609cf/raw/7ed3903084c6f309a5fe4456512dfb473504e919/Custom%20deep%20copy%20function%20using%20recursion.js

I found it in this blog heh: https://medium.com/javascript-in-plain-english/how-to-deep-copy-objects-and-arrays-in-javascript-7c911359b089 --> it's option 3

@mjc1283
Copy link
Contributor Author

mjc1283 commented Apr 21, 2020

@mikeng13 Yea, that'd probably work. But we know what the datafile schema is, we don't need to handle arbitrary objects. The datafile schema does not change very often, and when it does, we don't necessarily need to change this code. Overall I think the project config module could be a lot cleaner, but it would require careful refactoring of many other modules, so not trying to tackle it now. I think we can set it up so nothing is mutated, and we create fast lookup objects/Maps separately from the datafile.

@mjc1283
Copy link
Contributor Author

mjc1283 commented Apr 21, 2020

Put another way, copying/cloning is only required because we are mutating the datafile object. But there is no inherent need to mutate the datafile object. I think we can refactor so that it's nothing is mutated, and nothing is copied.

@mikeproeng37
Copy link
Contributor

Hmm.. I think even if we don't mutate the datafile, customers could end up inadvertently mutating it and causing issues. I think cloning it for safe use makes sense.

Copy link
Contributor

@mikeproeng37 mikeproeng37 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mjc1283
Copy link
Contributor Author

mjc1283 commented Apr 21, 2020

That's a fair point (cloning for safety). I didn't look too closely at the Medium post. Earlier I had my eye on this npm library rfdc. I think we could include a forked snippet from that (don't need all its features) and would only add about 50-100 LOC.

@jamesopti
Copy link
Contributor

That's a fair point (cloning for safety). I didn't look too closely at the Medium post. Earlier I had my eye on this npm library rfdc. I think we could include a forked snippet from that (don't need all its features) and would only add about 50-100 LOC.

This would be my preference in the medium term, but the current fix is OK by me for now!
Could that utility also replace our Object.assign polyfill?

@mjc1283 mjc1283 removed the WIP label Apr 22, 2020
@mjc1283 mjc1283 merged commit 862408c into master Apr 22, 2020
@mjc1283 mjc1283 deleted the mcarroll/OASIS-6301 branch April 22, 2020 16:49
jasonkarns added a commit to github/optimizely-javascript-sdk that referenced this pull request May 12, 2020
* v4:
  test suite doesn't pass cleanly
  prepare already runs build
  v4.0.0-github.0
  Ignore package tarballs
  Resume building non-minified ES entrypoint
  revert to master
  Expose an ESM-FULL bundle which includes optimizely deps
  Standardize ESM bundle
  Generate both min and non-min esm output
  exports option for es output is redundant. auto is best
  Use rollup --config- options for specifying bundles
  Build-all by default
  build various bundles using --config-* param
  chore: Prepare for 4.0.0 release (optimizely#468)
  fix: Removed React Native client engine temporarily (optimizely#466)
  chore: Prepare for 4.0.0-rc.2 release (optimizely#465)
  chore: Add source maps to build output (optimizely#464)
  fix(project config): Don't mutate the datafile object (optimizely#462)
  chore: Prepare for 4.0.0-rc.1 release (optimizely#461)
  chore: Prepare for js-sdk-datafile-manager 0.5.0 release (optimizely#460)
  chore(datafile manager): Remove react native datafile manager entry from package.json (optimizely#459)
  fix(datafile manager): Node datafile requests use gzip,deflate compression (optimizely#456)
  docs(datafile manager): Removed manual dependency installation requirement from docs (optimizely#457)
  chore(datafile manager): Added async storage as dev dependency (optimizely#455)
  refactor: Added a separate bundle for Json Schema Validator (optimizely#454)
  refactor: Convert lib/utils to ES module (Part 2/2) (optimizely#452)
  refactor: Convert lib/core to ES module (Part 2/2) (optimizely#450)
  chore(datafile manager): Update years in header comments (optimizely#453)
  refactor: Convert lib/core to ES module (Part 1/2) (optimizely#449)
  refactor: Convert lib/utils to ES module (Part 1/2) (optimizely#451)
  refactor: Convert lib/optimizely/* and lib/tests/* to ES module (optimizely#448)
  refactor: Convert lib/plugins from CJS to ES module (optimizely#427)
  chore: Update js-sdk-utils to 0.2.0 (optimizely#447)
  refactor: Convert entry points to es module and create minified bundles (optimizely#445)
  chore: Update CHANGELOG.MD for jsonSchemaValidator change (optimizely#443)
  Change functionality of JSON schema validation by removing skipJSONValidation entirely. Now a user needs to import jsonSchemaValidator from @optimizely/optimizely-sdk/lib/utils/json_schema_validator and pass it to createInstance when validation is desired. (optimizely#442)
  chore(datafile manager): Lint and formatting fixes for datafile manager tests (optimizely#441)
  [OASIS-6102]: changed functionality of the JSON schema validator module (optimizely#438)
  chore(datafile manager): Fix ESLint warnings & errors, apply Prettier formatting (optimizely#440)
  fix (datafile-manager): Fix boolean types, remove StaticDatafileManager, update READMe, add eslint + prettier (optimizely#436)
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.

3 participants