Skip to content

Update types for upcoming changes #22

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Update types for upcoming changes #22

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 42 additions & 2 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,54 @@ const { socketYmlSchemaV1 } = require('./lib/v1')
* @property {boolean} [authenticatedProjectReports] enable/disable authenticated project report URLs
*/

/**
* Configures how an issue should be handled by a given layer of configuration.
* - "defer" will cause the issue to bubble up to a higher layer. E.G. socket.yml may defer to organization settings. This acts as if the value is unset.
* - "error" will cause an alert to be shown and require some kind of interaction/setting and will block usage if it is encountered. The interaction may be a prompt, a PR comment, etc.
* - "warn" will cause an alert to be shown but will not require any interaction/setting and will not block usage if it is encountered.
* - "ignore" will cause an issue to not be shown and not require any interaction/setting and will not block usage if it is encountered.
*
* @typedef {'defer' | 'error' | 'warn' | 'ignore'} SocketIssueRuleAction
*/

/**
* @typedef SocketIssueRuleObject
* @property {SocketIssueRuleAction} [action]
*/

/**
* @typedef {boolean | SocketIssueRuleObject} SocketIssueRuleValue
Copy link
Member

Choose a reason for hiding this comment

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

A few thoughts after mulling over it. Curious what your thoughts are:

Did you consider something more like

Suggested change
* @typedef {boolean | SocketIssueRuleObject} SocketIssueRuleValue
* @typedef {boolean | 'defer' | 'error' | 'warn' | 'ignore'} SocketIssueRuleValue

Just thinking

issueRules:
  - didYouMean: 'warn'
  - installScript: 'error'

Is easier to remember than

  - didYouMean: 
       action: 'warn'
  - installScript: 
       action: 'error'

If we have plans for additional keys, maybe the object makes sense. Do we have an idea what those would be and how they would be used?

The other thought I had was what was the purpose of the defer rule action? error and warn take the place of true, ignore is a synonym for false. But omitting a rule is implicitly a defer. Does it enable some kind of new functionality or just serve the role of explicitly defining that behavior?

Copy link
Author

Choose a reason for hiding this comment

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

We definitely should look at telemetry and role based prompting here which is why I'm not keen on a string, if we do a string now it would be more breaking in the future. defer is to allow explicit saving of that so that the behavior is recorded and not changed. Some UI toggling for example of true/false with a checkbox cannot represent this currently and has an implicit (very hard to understand) 3rd state that is like defer going on right now.

*/

/**
* @typedef SocketYml
* @property {2} version
* @property {string[]} projectIgnorePaths
* @property {{ [issueName: string]: boolean }} issueRules
* @property {{ [issueName: string]: SocketIssueRuleValue }} issueRules
* @property {SocketYmlGitHub} githubApp
*/

/** @type {import('ajv').JSONSchemaType<SocketIssueRuleValue>} */
const socketYmlIssueRule = {
anyOf: [
{
type: 'boolean',
default: false
},
{
type: 'object',
properties: {
action: {
Copy link
Member

Choose a reason for hiding this comment

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

Is the plan to add more properties to this?

type: 'string',
enum: ['defer', 'error', 'warn', 'ignore'],
// note: ajv does not allow this enum to be `null`, this is due to anyOf
nullable: true
}
}
}
]
}

/** @type {import('ajv').JSONSchemaType<SocketYml>} */
const socketYmlSchema = {
$schema: 'http://json-schema.org/draft-07/schema#',
Expand All @@ -40,7 +80,7 @@ const socketYmlSchema = {
issueRules: {
type: 'object',
required: [],
additionalProperties: { type: 'boolean' },
additionalProperties: socketYmlIssueRule,
default: {}
},
githubApp: {
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
"build:2-schema-file": "echo \"console.log(JSON.stringify(require('.').socketYmlSchema, undefined, 2))\" | node > schema.json",
"build": "run-s build:*",
"check:dependency-check": "dependency-check '*.js' 'test/**/*.js' --no-dev",
"check:installed-check": "installed-check -i eslint-plugin-jsdoc",
"//check:installed-check": "installed-check -i eslint-plugin-jsdoc",
"check:lint": "eslint --report-unused-disable-directives .",
"check:tsc": "tsc",
"check:type-coverage": "type-coverage --detail --strict --at-least 95 --ignore-files 'test/*'",
Expand Down
6 changes: 5 additions & 1 deletion tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,20 @@
"test/**/*",
],
"compilerOptions": {
"composite": true,

"allowJs": true,
"checkJs": true,
"noEmit": true,
"resolveJsonModule": true,
"module": "es2022",
"moduleResolution": "node",

/* New checks being tried out */
"strictNullChecks": true,
"exactOptionalPropertyTypes": true,
"noFallthroughCasesInSwitch": true,
"noImplicitAny": true,
"noImplicitReturns": true,
"noImplicitOverride": true,
"noPropertyAccessFromIndexSignature": true,
"noUncheckedIndexedAccess": true,
Expand Down