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

feat(validation): allow additionalProperties (closes #401) #411

Merged
merged 15 commits into from
Mar 3, 2022

Conversation

gadicc
Copy link
Owner

@gadicc gadicc commented Feb 24, 2022

Closes #401.

Not done yet but in process.

We need to make a distinction between Options and Results, since we do actually rely on { additionalProperties: false } to validate the options the user is passing to various modules (and ensure e.g. they didn't mispell anything).

Should be fine to simply traverse the tree and apply additionalProperties on interfaces with names ending Options (while remembering there's a module called Options too, and see if there are any other naming conflicts).

@gadicc
Copy link
Owner Author

gadicc commented Feb 24, 2022

Self note: rather than making this a schema compile time option across most/all definitions, let's:

  • Carefully add [key: string]: any only to relevant interfaces in TS
  • Have a runtime option to remove these from the schema

@gadicc gadicc force-pushed the additionalProps branch from 0abb7a4 to e58638d Compare March 2, 2022 10:28
@codecov
Copy link

codecov bot commented Mar 2, 2022

Codecov Report

Merging #411 (5ee8843) into devel (954f379) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             devel      #411   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines          451       461   +10     
  Branches       142       146    +4     
=========================================
+ Hits           451       461   +10     
Impacted Files Coverage Δ
src/modules/chart.ts 100.00% <ø> (ø)
src/modules/historical.ts 100.00% <ø> (ø)
src/modules/insights.ts 100.00% <ø> (ø)
src/modules/options.ts 100.00% <ø> (ø)
src/modules/quote.ts 100.00% <ø> (ø)
src/modules/recommendationsBySymbol.ts 100.00% <ø> (ø)
src/modules/search.ts 100.00% <ø> (ø)
src/modules/trendingSymbols.ts 100.00% <ø> (ø)
src/lib/validateAndCoerceTypes.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 954f379...5ee8843. Read the comment docs.

@gadicc
Copy link
Owner Author

gadicc commented Mar 3, 2022

For those following, here's the new section in the validation docs:

A Note on "Additional Properties"

Since v2.2 (March 2022)

We now allow "additional properties" on results received from Yahoo.

This means that while we can still guarantee the type of all known fields we
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:

yahooFinance._disallowAdditionalProps();

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.

@gadicc gadicc changed the title fix(validation): allow additionalProperties (closes #401) feat(validation): allow additionalProperties (closes #401) Mar 3, 2022
@gadicc
Copy link
Owner Author

gadicc commented Mar 3, 2022

I feel like this is low risk and can help prevent future unnecessary breakages in existing deployments, so I'm going to go ahead and merge and release.

@gadicc gadicc merged commit 00f4f8e into devel Mar 3, 2022
gadicc pushed a commit that referenced this pull request Mar 3, 2022
# [2.2.0](v2.1.9...v2.2.0) (2022-03-03)

### Features

* **validation:** allow additionalProperties (closes [#401](#401)) ([#411](#411)) ([00f4f8e](00f4f8e))
@gadicc
Copy link
Owner Author

gadicc commented Mar 3, 2022

🎉 This PR is included in version 2.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@gadicc gadicc added the released label Mar 3, 2022
@gadicc gadicc deleted the additionalProps branch March 3, 2022 16:32
@pudgereyem
Copy link
Contributor

@gadicc awesome work!

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

Successfully merging this pull request may close these issues.

Consider allowing additionalProperties in Yahoo Results
2 participants