Make urlCache as a part of load context (#707) #708

mbelsky · 2021-08-02T01:33:56Z

Closes #707

ericzorn93 · 2021-08-09T14:08:03Z

src/load.ts

 /** Load a schema from local path or remote URL */
 export default async function load(
  schema: URL | PartialSchema,
  options: LoadOptions
 ): Promise<{ [url: string]: PartialSchema }> {
+  const urlCache = options.urlCache || new Set<string>();


How can we ensure that the cache object being passed in is of type Set when the user is consuming the library with vanilla JS? They will have types provided, but it will not throw an error if they pass a different data structure.

Thanks for asking. As I see this function is not a part of the library public api. And the function uses the cache object for recursive calls optimization. Also as you may see the function does not check types of the other fields of LoadOptions.

codecov · 2021-08-12T14:58:37Z

Codecov Report

Merging #708 (e78959e) into main (56cc872) will decrease coverage by 2.18%.
The diff coverage is 55.55%.

@@            Coverage Diff             @@
##             main     #708      +/-   ##
==========================================
- Coverage   96.34%   94.15%   -2.19%     
==========================================
  Files          13       13              
  Lines         547      565      +18     
  Branches      194      198       +4     
==========================================
+ Hits          527      532       +5     
- Misses         19       32      +13     
  Partials        1        1

Impacted Files	Coverage Δ
src/load.ts	`82.50% <40.00%> (-8.93%)`	⬇️
src/index.ts	`96.66% <100.00%> (ø)`
src/transform/schema.ts	`99.00% <100.00%> (+0.01%)`	⬆️
src/utils.ts	`95.00% <100.00%> (-1.56%)`	⬇️

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 72acb8d...e78959e. Read the comment docs.

mbelsky · 2021-08-13T20:20:04Z

Hey @drwpow

I've made two tests runs on my local machine: one on the main branch and one on the pr/pass-cache-in-context. And the tests run time difference is about 100ms. So I'm not sure how may I optimize the pr changes to make the tests pass. If you have any ideas about this issue I would like to hear about it.

Run tests on the main branch

~/gh/mb-openapi-ts (main)$ npm test -- ./tests/v3/index.test.ts 

> [email protected] test
> npm run build && jest --no-cache "./tests/v3/index.test.ts"


> [email protected] build
> rm -rf dist && tsc --project tsconfig.esm.json && tsc --project tsconfig.cjs.json

 PASS  tests/v3/index.test.ts (48.591 s)
  cli
    ✓ reads github.yaml spec (v3) from file (4074 ms)
    ✓ reads github.yaml spec (v3) from file (additional properties) (4303 ms)
    ✓ reads github.yaml spec (v3) from file (immutable types) (4329 ms)
    ✓ reads manifold.yaml spec (v3) from file (1075 ms)
    ✓ reads manifold.yaml spec (v3) from file (additional properties) (1076 ms)
    ✓ reads manifold.yaml spec (v3) from file (immutable types) (1064 ms)
    ✓ reads null-in-enum.yaml spec (v3) from file (694 ms)
    ✓ reads null-in-enum.yaml spec (v3) from file (additional properties) (698 ms)
    ✓ reads null-in-enum.yaml spec (v3) from file (immutable types) (695 ms)
    ✓ reads petstore-openapitools.yaml spec (v3) from file (830 ms)
    ✓ reads petstore-openapitools.yaml spec (v3) from file (additional properties) (829 ms)
    ✓ reads petstore-openapitools.yaml spec (v3) from file (immutable types) (824 ms)
    ✓ reads petstore.yaml spec (v3) from file (821 ms)
    ✓ reads petstore.yaml spec (v3) from file (additional properties) (830 ms)
    ✓ reads petstore.yaml spec (v3) from file (immutable types) (832 ms)
    ✓ reads stripe.yaml spec (v3) from file (3238 ms)
    ✓ reads stripe.yaml spec (v3) from file (additional properties) (3899 ms)
    ✓ reads stripe.yaml spec (v3) from file (immutable types) (3711 ms)
    ✓ reads spec (v3) from remote resource (1196 ms)
  json
    ✓ reads github.yaml from JSON (6150 ms)
    ✓ reads manifold.yaml from JSON (235 ms)
    ✓ reads null-in-enum.yaml from JSON (33 ms)
    ✓ reads petstore-openapitools.yaml from JSON (82 ms)
    ✓ reads petstore.yaml from JSON (87 ms)
    ✓ reads stripe.yaml from JSON (3399 ms)

Test Suites: 1 passed, 1 total
Tests:       25 passed, 25 total
Snapshots:   0 total
Time:        48.72 s
Ran all test suites matching /.\/tests\/v3\/index.test.ts/i.

Run tests on the pr/pass-cache-in-context branch

~/gh/mb-openapi-ts (pr/pass-cache-in-context)$ npm test -- ./tests/v3/index.test.ts 

> [email protected] test
> npm run build && jest --no-cache "./tests/v3/index.test.ts"


> [email protected] build
> rm -rf dist && tsc --project tsconfig.esm.json && tsc --project tsconfig.cjs.json

 PASS  tests/v3/index.test.ts (48.653 s)
  cli
    ✓ reads github.yaml spec (v3) from file (4115 ms)
    ✓ reads github.yaml spec (v3) from file (additional properties) (4447 ms)
    ✓ reads github.yaml spec (v3) from file (immutable types) (4273 ms)
    ✓ reads manifold.yaml spec (v3) from file (902 ms)
    ✓ reads manifold.yaml spec (v3) from file (additional properties) (946 ms)
    ✓ reads manifold.yaml spec (v3) from file (immutable types) (940 ms)
    ✓ reads null-in-enum.yaml spec (v3) from file (610 ms)
    ✓ reads null-in-enum.yaml spec (v3) from file (additional properties) (616 ms)
    ✓ reads null-in-enum.yaml spec (v3) from file (immutable types) (608 ms)
    ✓ reads petstore-openapitools.yaml spec (v3) from file (739 ms)
    ✓ reads petstore-openapitools.yaml spec (v3) from file (additional properties) (724 ms)
    ✓ reads petstore-openapitools.yaml spec (v3) from file (immutable types) (740 ms)
    ✓ reads petstore.yaml spec (v3) from file (754 ms)
    ✓ reads petstore.yaml spec (v3) from file (additional properties) (827 ms)
    ✓ reads petstore.yaml spec (v3) from file (immutable types) (841 ms)
    ✓ reads stripe.yaml spec (v3) from file (3809 ms)
    ✓ reads stripe.yaml spec (v3) from file (additional properties) (4025 ms)
    ✓ reads stripe.yaml spec (v3) from file (immutable types) (3712 ms)
    ✓ reads spec (v3) from remote resource (1226 ms)
  json
    ✓ reads github.yaml from JSON (6001 ms)
    ✓ reads manifold.yaml from JSON (224 ms)
    ✓ reads null-in-enum.yaml from JSON (27 ms)
    ✓ reads petstore-openapitools.yaml from JSON (75 ms)
    ✓ reads petstore.yaml from JSON (83 ms)
    ✓ reads stripe.yaml from JSON (3258 ms)

Test Suites: 1 passed, 1 total
Tests:       25 passed, 25 total
Snapshots:   0 total
Time:        48.798 s
Ran all test suites matching /.\/tests\/v3\/index.test.ts/i.

kamsar · 2021-09-08T23:05:10Z

The lack of this fix really crippled my attempt to use the node API, since the cache lives forever - and does not actually add the cached schema to the second and later call, i.e.

if (urlCache.has(schemaID))
            return options.schemas;

In the above code in a second or later function invocation, urlCache will have the schema cached - but options.schemas will NOT contain it, resulting in spuriously unresolved externals in the output.

drwpow

Awesome fix! Thank you.

drwpow · 2021-09-30T21:22:20Z

@all-contributors please add @mbelsky for code, bug

allcontributors · 2021-09-30T21:22:28Z

@drwpow

I've put up a pull request to add @mbelsky! 🎉

drwpow · 2021-09-30T21:24:55Z

Released in 4.1.1!

kamsar · 2021-09-30T21:56:13Z

@drwpow thanks so much, really appreciate this fix :)

Make urlCache as a part of load context (#707)

826616c

ericzorn93 reviewed Aug 9, 2021

View reviewed changes

Merge branch 'main' into pr/pass-cache-in-context

e78959e

drwpow approved these changes Sep 30, 2021

View reviewed changes

drwpow merged commit 091b053 into openapi-ts:main Sep 30, 2021

allcontributors bot mentioned this pull request Sep 30, 2021

docs: add mbelsky as a contributor for code, bug #767

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make urlCache as a part of load context (#707) #708

Make urlCache as a part of load context (#707) #708

mbelsky commented Aug 2, 2021

ericzorn93 Aug 9, 2021

mbelsky Aug 9, 2021

codecov bot commented Aug 12, 2021 •

edited

Loading

mbelsky commented Aug 13, 2021

kamsar commented Sep 8, 2021

drwpow left a comment

drwpow commented Sep 30, 2021

allcontributors bot commented Sep 30, 2021

drwpow commented Sep 30, 2021

kamsar commented Sep 30, 2021

Make urlCache as a part of load context (#707) #708

Make urlCache as a part of load context (#707) #708

Conversation

mbelsky commented Aug 2, 2021

ericzorn93 Aug 9, 2021

Choose a reason for hiding this comment

mbelsky Aug 9, 2021

Choose a reason for hiding this comment

codecov bot commented Aug 12, 2021 • edited Loading

Codecov Report

mbelsky commented Aug 13, 2021

kamsar commented Sep 8, 2021

drwpow left a comment

Choose a reason for hiding this comment

drwpow commented Sep 30, 2021

allcontributors bot commented Sep 30, 2021

drwpow commented Sep 30, 2021

kamsar commented Sep 30, 2021

codecov bot commented Aug 12, 2021 •

edited

Loading