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

Generate FidesJS SDK Reference Docs from tsdoc comments #4736

Merged
merged 50 commits into from
Mar 27, 2024

Conversation

NevilleS
Copy link
Contributor

@NevilleS NevilleS commented Mar 21, 2024

Closes PROD-1815

Description Of Changes

This establishes a new directory, src/docs, which is meant to act as the external-facing type docs for using FidesJS. These types are heavily commented using TSDoc and then converted into Markdown docs using Typedoc to populate the top-level docs/ folder 👍. As a final step, you can then run npm run docs:import in our fidesdocs project to import these docs into the FidesJS SDK Reference section here: https://ethyca.com/docs/dev-docs/js/reference/README

The bulk of the changes here are all about renaming and cleaning up some of the types used within the fides-js project to be more clear. As much as possible I've taken the strategy of having our types extend the src/docs/... types so that we can be more specific in the code than what is reflected in the dev docs.

Code Changes

The most significant change here is some renaming of a bunch of types that I felt had lost some of their meaning...

Before After Why?
CookieKeyConsent NoticeConsent The Fides.consent object isn't really about the cookie: it's a map of notice_key -> `true
CookieIdentity FidesJSIdentity Similar to above - the fact that we save this to a cookie isn't the defining characteristic here. We also pass this identity object around to events, APIs, etc. I considered FidesIdentity instead of FidesJSIdentity but felt like reminding us that this was the structure used in the JS library felt helpful...?
CookieMeta FidesJSMeta Same as above.
Fides FidesGlobal Debatable, honestly, but Fides was such an interchangeable type in this codebase that it felt like FidesGlobal at least hinted at what it was used for. I reserved the Fides interface name for the documented interface and ended up doing a little export interface FidesGlobal extends Fides { ... } in the code
OverrideOptions FidesOptions This is a potentially confusing "rebrand". I felt like we needed to state that the "Fides Options" were the documented options that could be set at runtime via our fides_overrides object / query params / etc. To an engineer integrating FidesJS into their website, those are the options available to them, so calling them "overrides" felt overly intimidating.
FidesOptions FidesInitOptions This follows from above and "frees up" the FidesOptions name for the documented SDK 👍
FidesOptionsOverrides FidesInitOptionsOverrides Same as above - this is a utility type for our purposes, so let's reserve the "FidesOptions" term for our customers
  • Rename several Typescript types (see above)
  • Add typedoc library to generate docs from tsdoc comments in ./src/docs to ./docs
  • Add a docs:generate npm task to generate docs, and run that after every build

Pre-Merge Checklist

Copy link

vercel bot commented Mar 21, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
fides-plus-nightly ⬜️ Ignored (Inspect) Visit Preview Mar 27, 2024 6:55pm

Copy link

cypress bot commented Mar 21, 2024

Passing run #6909 ↗︎

0 4 0 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge df2609c into 0946885...
Project: fides Commit: 29ec7542d8 ℹ️
Status: Passed Duration: 00:34 💡
Started: Mar 27, 2024 6:41 PM Ended: Mar 27, 2024 6:42 PM

Review all test suite changes for PR #4736 ↗︎

@NevilleS NevilleS changed the title FidesJS SDK Reference Docs Generate FidesJS SDK Reference Docs from Typescript docs comments Mar 27, 2024
@NevilleS NevilleS changed the title Generate FidesJS SDK Reference Docs from Typescript docs comments Generate FidesJS SDK Reference Docs from tsdoc comments Mar 27, 2024
Copy link

codecov bot commented Mar 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.64%. Comparing base (e4ec5f7) to head (5a36077).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4736   +/-   ##
=======================================
  Coverage   86.64%   86.64%           
=======================================
  Files         338      338           
  Lines       19993    19993           
  Branches     2555     2555           
=======================================
  Hits        17323    17323           
  Misses       2201     2201           
  Partials      469      469           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

export type TcfCookieKeyConsent = {
[id: string | number]: boolean | undefined;
};
export type TcfSystemsConsent = Record<string | number, boolean>;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

import { FIDES_OVERRIDE_OPTIONS_VALIDATOR_MAP } from "./consent-constants";
import { setupExtensions } from "./extensions";
import {
noticeHasConsentInCookie,
transformConsentToFidesUserPreference,
} from "./shared-consent-utils";
import type { Fides } from "../docs";
Copy link
Contributor

Choose a reason for hiding this comment

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

Having a dependency on the docs in this part of the code is concerning. Can this typing be moved to a more generic/shared part of the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's fair - I was also surprised to find that we defined the "Fides Global" interface in this file and not our big "consent-types" file that has everything else.

Copy link
Contributor

@gilluminate gilluminate left a comment

Choose a reason for hiding this comment

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

Took another pass at it to take a closer look at renaming and TS changes. Overall LGTM, just 1 question on using the interface from the docs.

Copy link
Contributor

@eastandwestwind eastandwestwind left a comment

Choose a reason for hiding this comment

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

🚀 :shipit:


We use [Typedoc](https://typedoc.org/) to generate "FidesJS SDK Reference" docs that explain how to use the `fides.js` script in a customer website. You can find those docs here: [docs/README.md](./docs/README.md)

To regenerate the developer docs, run `npm run docs:generate`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, thanks for adding this!

@eastandwestwind
Copy link
Contributor

Do you know why this Fides init type declaration description has "something" in it? Is that like the default for Typedoc?

Screenshot 2024-03-27 at 5 58 39 PM

@NevilleS
Copy link
Contributor Author

NevilleS commented Mar 27, 2024

Do you know why this Fides init type declaration description has "something" in it? Is that like the default for Typedoc?

Screenshot 2024-03-27 at 5 58 39 PM

That's coming from the types in src/docs/fides.ts which I had a messy doc comment saying "something" was the description for that param: 29906fa#diff-b7d497dc581d7234aba132f1f03d59d7defb94e9b36385af4715034aceab9e39L181

For these docs I felt like documenting the details of the config object didn't make sense, so I'm leaving it as any and nonspecific.

Once I re-run docs:generate that'll get fixed up, just need to get Typescript compilation clean again!

@NevilleS NevilleS merged commit 6e9aa4f into main Mar 27, 2024
6 of 7 checks passed
@NevilleS NevilleS deleted the PROD-1815-ns-fides-js-docs branch March 27, 2024 18:55
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