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.

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

Migrate Validation to Typebox #772

Merged
merged 13 commits into from
Aug 7, 2024
Merged

Migrate Validation to Typebox #772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
8257099
Add YFinance Typebox types + validation & coercion
eddie-atkinson May 4, 2024
81cb253
Add Typebox search module
eddie-atkinson May 11, 2024
c008cca
Add QuoteSummary Typebox module
eddie-atkinson Jun 4, 2024
9062fa4
Add Screener Typebox module
eddie-atkinson Jun 4, 2024
4b4efee
Add RecommendationBySymbol module
eddie-atkinson Jun 4, 2024
38e4bd3
Add Options Typebox Module
eddie-atkinson Jul 21, 2024
f67181d
Add Insights Typebox Module
eddie-atkinson Jul 21, 2024
99b089a
Add Historical module
eddie-atkinson Jul 21, 2024
ff19859
Add FundamentalsTimeSeries module
eddie-atkinson Jul 21, 2024
fc450b9
Adds DailyGainers Module
eddie-atkinson Jul 21, 2024
8805004
Adds Chart module
eddie-atkinson Jul 21, 2024
873a0a5
Add quote typebox module, migrate quoteCombine and move over options …
eddie-atkinson Jul 22, 2024
6af931d
Remove JSON schema validation pathway entirely
eddie-atkinson Jul 26, 2024
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
3 changes: 0 additions & 3 deletions .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,6 @@ jobs:
# Check for TypeScript errors
- run: yarn test:ts

# Check that schema is up-to-date
- run: scripts/schema-check.sh

- run: yarn prettier --check src tests tests-modules

# run tests!
Expand Down
16 changes: 5 additions & 11 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,6 @@ work and submitting your PR.

All PRs should be submitted against the `devel` branch (github default).

**Changing TypeScript Interfaces**

Run `yarn generateSchema` after changing any interfaces, this will regenerate
the `schema.json` file which is used for run-time tests.

**Commit Messages**

Commit messages should follow the
Expand Down Expand Up @@ -134,12 +129,11 @@ TODO
Checklist:

1. Create it in `src/modules/myAmazingModule.ts`
1. Run `yarn generateSchema` (and on any future interface changes)
1. Test it in `src/modules/myAmazingModule.spec.ts`
1. Add it to `src/index-common.ts`
1. Docs in `docs/modules/myAmazingModule.md`
1. Link these docs in `README.md` and `docs/README.md`.
1. Commit all the above and any `tests/http/*` created in your tests.
2. Test it in `src/modules/myAmazingModule.spec.ts`
3. Add it to `src/index-common.ts`
4. Docs in `docs/modules/myAmazingModule.md`
5. Link these docs in `README.md` and `docs/README.md`.
6. Commit all the above and any `tests/http/*` created in your tests.

For a model example, see the
[recommendationsBySymbol PR](https://github.com/gadicc/node-yahoo-finance2/pull/28)
Expand Down
5 changes: 3 additions & 2 deletions docs/validation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,16 +146,17 @@ receive back, we'll no longer throw a new error on new unknown keys.
This is important because Yahoo constantly add new fields, and this would
break all existing deployments.

You can revert to the old behaviour with:
You can revert to the old behaviour by passing the `_internalThrowOnAdditionalProperties` to the global configuration like so:

```js
yahooFinance._disallowAdditionalProps();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gadicc I guess this is probably a breaking change given the docs previously stated users could use this if required.

Note: We can support this if desired, by having this as an option on the config object to strip extraneous properties and then using that config value to run Clean from typebox to trim the result sets.

Though I'm not sure I understand the use case for wanting to throw if the payload has extraneous properties but otherwise matches our validation spec

Copy link
Owner

Choose a reason for hiding this comment

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

This is something I would actually like to keep, in one form or another. Doesn't necessarily need identical API since we prefixed with _.

  • For the user, most will never need this unless they're pedantic. We don't need to cover this.

  • For contributors, this has been the most useful way to see if we're adequately covering the entire response (that's why it's the default for NODE_ENV=test. I don't really want to accept commits unless they actually provide full typing for everything Yahoo is supplying - otherwise tests should fail.

So, the big requirement here is more for test validation during development rather than regular runtime validation of the library.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok not an issue.

I've added this as an option to the options.validation field and set it to default to NODE_ENV === "test" so that covers off the need for stricter types during development, whilst giving the options for callers to specify it themselves where it's useful to them with the caveat that it's an internal only option.

Let me know if this is sufficient to support all parties, if not I can have a bit more of a think about it

Copy link
Owner

Choose a reason for hiding this comment

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

Brilliant, yes, that's absolutely perfect! 🙏

yahooFinance.setGlobalConfig({ validation: { _internalThrowOnAdditionalProperties: true} });
```

which is the default when `NODE_ENV==="test"`. This means that during our
development of the library itself, we make sure that we're testing against
all types.


<a name="help-fix"></a>
## Help Fix Validation Errors

Expand Down
15 changes: 3 additions & 12 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,10 @@
"scripts": {
"coverage": "yarn test --coverage",
"lint": "eslint . --ext .js,.ts",
"//schema": "ts-json-schema-generator -f tsconfig.json -p 'src/{modules/**/*.ts,lib/options.ts}' -t '*' | node bin/schema-tweak.js > schema.json",
"schema": "node --loader ts-node/esm scripts/schema.js > schema.json",
"timeseries": "node --loader ts-node/esm scripts/timeseries.js",
"build": "yarn run build:esm && yarn run build:cjs && yarn run build:post",
"build": "yarn run build:esm && yarn run build:cjs",
"build:esm": "tsc --module es2020 --target es2019 --outDir dist/esm",
"build:cjs": "tsc --module commonjs --target es2015 --outDir dist/cjs && sed 's/\"type\": \"module\",/\"type:\": \"commonjs\",/' dist/cjs/package.json > dist/cjs/package-changed.json && mv dist/cjs/package-changed.json dist/cjs/package.json",
"build:post": "scripts/json-transform.sh",
"generateSchema": "yarn schema",
"prepublishOnly": "yarn build && yarn generateSchema && yarn build:post",
"test": "node --experimental-vm-modules ./node_modules/jest/bin/jest.js",
"test:ts": "tsc --noEmit",
"test:esm": "node --experimental-vm-modules ./node_modules/jest/bin/jest.js -c tests-modules/esm/jest.config.js tests-modules/esm/tests/*",
Expand All @@ -54,13 +49,11 @@
"test:build": "yarn test:modules"
},
"files": [
"dist",
"schema.json"
"dist"
],
"dependencies": {
"@sinclair/typebox": "^0.32.27",
"@types/tough-cookie": "^4.0.2",
"ajv": "8.10.0",
"ajv-formats": "2.1.1",
"node-fetch": "^2.6.1",
"tough-cookie": "^4.1.2",
"tough-cookie-file-store": "^2.0.3"
Expand All @@ -83,11 +76,9 @@
"globby": "13.2.2",
"jest": "29.7.0",
"jest-tobetype": "1.2.3",
"oas-schema-walker": "1.1.5",
"prettier": "2.8.8",
"semantic-release": "19.0.5",
"ts-jest": "29.1.2",
"ts-json-schema-generator": "1.5.0",
"ts-node": "10.9.2",
"typescript": "5.4.3"
}
Expand Down
Loading