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

Implement Scrubber for end-user data in thrown exceptions #4999

Merged

Conversation

alexandercampbell
Copy link
Contributor

@alexandercampbell alexandercampbell commented May 19, 2022

Description
This PR implements a new Scrubber module within Slate that can optionally be used to scrub exception contents of end-user data. The API change is backwards-compatible and the default behavior is unchanged. The docs/api/scrubber.md file in the diff includes additional detail.

This PR also fixes a handful of exceptions which were not stringifying the objects that were being thrown. Before this change, they would have thrown messages of the form: Cannot apply a "merge_node" operation at path [1,2,3] to nodes of different interfaces: [object Object]. After this change, those exceptions include a properly stringified representation of the object in question. See also the discussion in Slack of those exceptions.

Issue
There is no corresponding Github issue. I'd be happy to create one if that's preferred.

Example
Here's an example of scrubbing the 'text' field of any entity that gets logged as part of an exception.

import { Scrubber } from 'slate'

Scrubber.setScrubber((key, value) => {
  if (key === 'text') return '... scrubbed ...'
  return value
})

Context

  • The Scrubber module requires one global config var. This had to be done because many of the Slate functions do not take an Editor parameter (e.g,Node.fragment(node, path)).
  • The scrubbing is done by simply passing the scrubber function as the second parameter to JSON.stringify when generating exception strings.
  • I have written some documentation under docs/api for this new feature. Please let me know if I have missed some context on how those docs should be written or formatted.
  • I have created a unit test to verify the behavior of the provided Scrubber.setScrubber function.
  • See also the Slack discussion in #contributing from April 11 (and continuing through the 14th) for full discussion context.

Checks

  • The new code matches the existing patterns and styles.
  • The tests pass with yarn test.
  • The linter passes with yarn lint. (Fix errors with yarn fix.)
  • The relevant examples still work. (Run examples with yarn start.)
  • You've added a changeset if changing functionality. (Add one with yarn changeset add.)

@changeset-bot
Copy link

changeset-bot bot commented May 19, 2022

🦋 Changeset detected

Latest commit: 7846ca4

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

This PR includes changesets to release 2 packages
Name Type
slate Minor
slate-react 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

@cmditch
Copy link
Contributor

cmditch commented May 19, 2022

We've tested this in our codebase as well
image

*
* Scrubber.setScrubber(Scrubber.textRandomizer(['text']));
*/
textRandomizer(fieldNames: string[]): Scrubber {
Copy link
Collaborator

Choose a reason for hiding this comment

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

While I appreciate the addition of a helper function, the way it's implemented as an object property it can't be tree shaken when unused.

I don't think you need the ScrubberInterface and this sub-object at all; if you unwrap it and just export const the three functions inside it should be an equivalent interface.

Copy link
Contributor

@cmditch cmditch May 24, 2022

Choose a reason for hiding this comment

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

We thought we'd keep in line with how other Slate modules look (e.g., Transforms.foo, Editor.bar) so we could get Scrubber.setScrubber. Also made documentation a bit easier as we could just add a "Scrubber" section to the docs.

But we're fine with exporting them at the root level too, so you get something like:

import { Editor, Transforms, Node, Element, setScrubber, textRandomizer, createEditor  } from 'slate'

Copy link
Contributor

Choose a reason for hiding this comment

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

We could also keep it mostly as is and either remove textRandomizer (maybe just include it in the docs as an example of what one might do) or move it outside of the Scrubber interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback! I've updated the branch to remove textRandomizer from the code, since it was just a convenience function anyway. I've instead included it as an example in the docs/api/scrubber.md documentation page.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess I'm used to using import * as Scrubber internally and forgot Slate was structured this way. As this is an internal API, however, it could still be used by integrators as import { Scrubber } from 'slate' with a change to how index.ts is structured.

Moving it to documentation is a good solution. Maybe I'll look at this later as a wider change to improve tree shaking within Slate.

@alexandercampbell alexandercampbell force-pushed the error-message-scrubber branch 2 times, most recently from 7d46bd3 to 8935b5d Compare May 25, 2022 23:02
@alexandercampbell alexandercampbell force-pushed the error-message-scrubber branch from 8935b5d to 7846ca4 Compare May 25, 2022 23:14
@dylans dylans merged commit fe13a8f into ianstormtaylor:main May 26, 2022
@github-actions github-actions bot mentioned this pull request May 26, 2022
@alexandercampbell alexandercampbell deleted the error-message-scrubber branch June 6, 2022 16:12
z2devil pushed a commit to z2devil/slate that referenced this pull request Dec 6, 2024
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.

4 participants