Skip to content
This repository has been archived by the owner on Jan 8, 2022. It is now read-only.

feat(*): replace ow with joi #56

Closed
wants to merge 1 commit into from
Closed

feat(*): replace ow with joi #56

wants to merge 1 commit into from

Conversation

KhafraDev
Copy link
Contributor

Please describe the changes this PR makes and why it should be merged:

Finally fixes #54 (ow hasn't supported ESM in at least 6 months, it likely never did).
To fix ESM, the package author is planning on moving to ESM only, which in turn will break cjs users (they can still use dynamic imports but it's a pretty terrible solution, plus I don't know if the minifier would recognize this and minify accordingly?). So the option was to alienate esm users, or alienate cjs. The best solution I could think of was to just ditch ow.

Another solution would be to remove validation entirely because Discord's documentation is good enough as a reference for users to use instead. Or someone could roll back changes between v0.7.0 and 0.8.x that causes ow to get used in SlashCommandBuilder?

Another user experiencing the same issue.

Status and versioning classification:

  • I know how to update typings and have done so, or typings don't need updating
  • This PR changes the library's interface (methods or parameters added) - changes the errors and likely breaks other things, however all tests pass
  • Code changes have been tested, or there are no code changes
  • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)

@codecov-commenter
Copy link

Codecov Report

Merging #56 (91b7b1d) into main (db39cc0) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main       #56   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           21        21           
  Lines          264       263    -1     
  Branches        33        33           
=========================================
- Hits           264       263    -1     
Impacted Files Coverage Δ
src/interactions/contextMenuCommands/Assertions.ts 100.00% <100.00%> (ø)
src/interactions/slashCommands/Assertions.ts 100.00% <100.00%> (ø)
...s/slashCommands/mixins/CommandChannelOptionBase.ts 100.00% <100.00%> (ø)
...ractions/slashCommands/mixins/CommandOptionBase.ts 100.00% <100.00%> (ø)
...s/slashCommands/mixins/CommandOptionWithChoices.ts 100.00% <100.00%> (ø)
...actions/slashCommands/mixins/NameAndDescription.ts 100.00% <100.00%> (ø)
src/messages/embed/Assertions.ts 100.00% <100.00%> (ø)
src/messages/embed/Embed.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db39cc0...91b7b1d. Read the comment docs.

@vladfrangu
Copy link
Member

The whole point of the module is to provide TS-and-JS side validation of the data, so you know ahead of time if your commands are valid.

I'd love to know how the errors reported by Joi look like. Does Joi report all validation errors encountered or just the first one?

@KhafraDev
Copy link
Contributor Author

KhafraDev commented Nov 1, 2021

I'd love to know how the errors reported by Joi look like. Does Joi report all validation errors encountered or just the first one?

When using objects, Joi can report multiple errors (not enabled in this pr yet), but not when using multiple asserts.

For example:

const schema = Joi.object().keys({
    a: Joi.string(), 
    b: Joi.number()
});

Joi.assert({a: 1, b: false}, schema, { abortEarly: false })

and the error is

    throw error;
    ^

Error [ValidationError]: {
  "b" [2]: false,
  "a" [1]: 1
}

[1] "a" must be a string
[2] "b" must be a number
    at Object.exports.process (\ts\node_modules\joi\lib\errors.js:184:16)
    at Object.internals.entry (\ts\node_modules\joi\lib\validator.js:150:26)
    at Object.exports.entry (\ts\node_modules\joi\lib\validator.js:27:30)
    at internals.Base.validate (\ts\node_modules\joi\lib\base.js:548:26)
    at Object.internals.assert (\ts\node_modules\joi\lib\index.js:225:27)
    at Object.assert (\ts\node_modules\joi\lib\index.js:102:19)
    at Object.<anonymous> (\ts\fff.cjs:9:5)
    at Module._compile (node:internal/modules/cjs/loader:1101:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
    at Module.load (node:internal/modules/cjs/loader:981:32) {
  _original: { a: 1, b: false },
  details: [
    {
      message: '"a" must be a string',
      path: [ 'a' ],
      type: 'string.base',
      context: { label: 'a', value: 1, key: 'a' }
    },
    {
      message: '"b" must be a number',
      path: [ 'b' ],
      type: 'number.base',
      context: { label: 'b', value: false, key: 'b' }
    }
  ]
}

@Khasms
Copy link
Contributor

Khasms commented Nov 1, 2021

Other validation libraries, such as zod or superstruct should be considered as well. My personal vote is for zod. In my opinion, zod's syntax is more concise and it also requires no additional options to be enabled to report all errors. The equivalent of the above example using zod is

const schema = z.object({
    a: z.string(),
    b: z.number()
});

schema.parse({ a: 1, b: false });

which throws

ZodError: [
  {
    "code": "invalid_type",
    "expected": "string",
    "received": "number",
    "path": [
      "a"
    ],
    "message": "Expected string, received number"
  },
  {
    "code": "invalid_type",
    "expected": "number",
    "received": "boolean",
    "path": [
      "b"
    ],
    "message": "Expected number, received boolean"
  }
]
    at new ZodError (E:\builders\builders-validation\node_modules\.pnpm\z[email protected]\node_modules\zod\src\ZodError.ts:140:5)
    at handleResult (E:\builders\builders-validation\node_modules\.pnpm\z[email protected]\node_modules\zod\src\types.ts:72:19)
    at ZodObject.ZodType.safeParse (E:\builders\builders-validation\node_modules\.pnpm\z[email protected]\node_modules\zod\src\types.ts:184:12)
    at ZodObject.ZodType.parse (E:\builders\builders-validation\node_modules\.pnpm\z[email protected]\node_modules\zod\src\types.ts:162:25)
    at Object.<anonymous> (E:\builders\builders-validation\index.ts:16:8)
    at Module._compile (node:internal/modules/cjs/loader:1101:14)
    at Module.m._compile (E:\builders\builders-validation\node_modules\.pnpm\t[email protected]_0cb88d80cb04d25b21fa3ec194608c65\node_modules\ts-node\src\index.ts:1371:23)
    at Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
    at Object.require.extensions.<computed> [as .ts] (E:\builders\builders-validation\node_modules\.pnpm\t[email protected]_0cb88d80cb04d25b21fa3ec194608c65\node_modules\ts-node\src\index.ts:1374:12)
    at Module.load (node:internal/modules/cjs/loader:981:32) {
  issues: [
    {
      code: 'invalid_type',
      expected: 'string',
      received: 'number',
      path: [Array],
      message: 'Expected string, received number'
    },
    {
      code: 'invalid_type',
      expected: 'number',
      received: 'boolean',
      path: [Array],
      message: 'Expected number, received boolean'
    }
  ],
  format: [Function (anonymous)],
  addIssue: [Function (anonymous)],
  addIssues: [Function (anonymous)],
  flatten: [Function (anonymous)]
}

@KhafraDev
Copy link
Contributor Author

Honestly I just chose the most popular validation library I could find and really couldn't care less which one replaces ow. If you make a PR in place the maintainers can choose which lib to use I guess.

@Lioness100
Copy link

I would love to see this merged!

@iCrawl
Copy link
Member

iCrawl commented Nov 30, 2021

We decided to go with zod, thanks for the PR and the initial kick-off for the discussion in the first place 👍

@iCrawl iCrawl closed this Nov 30, 2021
@KhafraDev
Copy link
Contributor Author

awesome!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ESM TypeError: Cannot read properties of undefined (reading 'minLength')
6 participants