-
Notifications
You must be signed in to change notification settings - Fork 14
remove all ts-ignore #788
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
base: main
Are you sure you want to change the base?
remove all ts-ignore #788
Conversation
* @typedef {import("./deviceApiCalls/__generated__/validators-ts").BaseRuntimeConfiguration} BaseRuntimeConfiguration | ||
* @typedef {{ | ||
* contentScope: import("@duckduckgo/privacy-configuration/schema/config").ConfigV4<number>; | ||
* userPreferences: BaseRuntimeConfiguration['userPreferences']; | ||
* userUnprotectedDomains: BaseRuntimeConfiguration['userUnprotectedDomains']; | ||
* }} RuntimeConfiguration | ||
* @typedef {import("../packages/device-api").DeviceApi} DeviceApi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can just define RuntimeConfiguration here, using some types from schemas, and some from privacy-configuration repo
"type": "object", | ||
"additionalProperties": false, | ||
"title": "RuntimeConfiguration", | ||
"title": "BaseRuntimeConfiguration", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now this is just used as a base type, and never directly
// Also we might not need zod anymore for defining runtime config? We should simply rely on the | ||
// C-S-S types. | ||
// @ts-ignore - TODO: C-S-S must be migrated to use the config from privacy-configuration | ||
const enabled = autofillEnabled(runtimeConfig); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isn't needed with my change, but in general if you need to silence a TS error just do it on a variable level, to prevent disabling the type-checker for the entire line, eg:
const runtimeConfigAsAny = /** @type {any} */(runtimeConfig)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure yeah, we talked it, thanks! Will update.
const contentScopeAsAny = /** @type {any} */ (contentScope); | ||
const cssUserPreferences = /** @type {any} */ (userPreferences); | ||
const processedConfig = processConfig(contentScopeAsAny, userUnprotectedDomains, cssUserPreferences); | ||
return isAutofillEnabledFromProcessedConfig(processedConfig); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here we interface with an external lib (css) where the types are incorrect. so, the temporary solution is to set the vars as any
instead of ts-ignore
/** | ||
* This casting is done to erase the 'unknown' type for `contentScope`, since it now | ||
* comes from the privacy-configuration repo | ||
*/ | ||
this._runtimeConfiguration = /** @type {RuntimeConfiguration} */ (/** @type {any} */ runtimeConfig); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is like the as unknown as <type>
trick in TS
/** | ||
* @param {GlobalConfig} globalConfig | ||
* @returns {{success: import('../__generated__/validators-ts').RuntimeConfiguration}} | ||
* @returns {{success: import('../../Settings.js').RuntimeConfiguration}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here we can import the type we made manually.
c057232
to
590dd7c
Compare
6ee99c8
to
f37b4b3
Compare
2fe6954
to
6626710
Compare
@shakyShane, @dbajpeyi Are you planning to continue working on this PR? |
Reviewer:
Asana:
Description
Steps to test