refactor: nodejs sdk design - dependencies - declaration to be more ts alike and up to date #365
Conversation
Pulsar-momentum
changed the title
…ch type + update openapi-fetch to latest
README.md
Outdated
| import OFF from "openfoodfacts-nodejs"; | ||
| const client = new OFF(); | ||
| import { OpenFoodFacts } from "openfoodfacts-nodejs"; | ||
| const client = new OFF({ country: 'string' || null }); |
There was a problem hiding this comment.
This should be an example, not the typing of the constructor.
| @@ -1,31 +1,33 @@ | |||
| { | |||
| "name": "openfoodfacts-nodejs", | |||
| "name": "openfoodfacts-nodejs-ts", | |||
There was a problem hiding this comment.
Why should we change the name of the package?
There was a problem hiding this comment.
I made the structure and the file working in and for ts, so in my point it make sense to rename it to "ts" to make people aware it's intended to also work for ts project
For what I know ts packages work for ts and js, but always make it explicit when they are made to work for ts
src/main.ts
Outdated
| const fill = "x".repeat(13 - beginning.length); | ||
| const barcode = beginning.concat(fill); | ||
| return this.getProduct(barcode); | ||
| this.baseUrl = 'https://world.openfoodfacts.org' |
There was a problem hiding this comment.
Why remove the {options.country} ?
There was a problem hiding this comment.
From my test (using fr country) it didn't seems to work, maybe I did it wrong, in case I will put it back
| export type Product = components['schemas']['Product'] | ||
| export type SearchResult = external['responses/search_for_products.yaml'] | ||
|
|
||
| export type Fetch = typeof fetch |
There was a problem hiding this comment.
I think this type could be removed. Is there any reason to keep it?
There was a problem hiding this comment.
Yes, by exemple in my own project (that is also why I m making this refacto) I need the Product type from the off sdk to be accessible cause I need to type my return function
My own product type (coming from the exported mongo database) isn't compatible with the sdk return so I need this type to make some data convertion
Yet it can be inferered from the return but making it visible is a good thing for some case as mine
| @@ -7,16 +7,18 @@ | |||
| ], | |||
| "module": "CommonJS", | |||
| "lib": [ | |||
| "ES2022" | |||
| "ES2022", | |||
| "dom", | |||
There was a problem hiding this comment.
We should not need DOM if we're working in pure NodeJS
There was a problem hiding this comment.
The NodeJS fetch type was in conflit with the openapi-fetch packages, in this they are using the fetch type coming from dom and not from NodeJS
Before the refactoring the project was shouting a warning about fetch incompatible type, so using the fetch type coming from dom was the solution to be aligned with openapi-fetch packages
…n read me to be usable and provide more explanation
|
|
Hello @VaiTon, I made some new change about what you pointed out, aside for the point that need some more discussion |
|
@VaiTon @Valetek arg, merge conflicts :-( |
|
@teolemon gave a look, I can't do the merge myself ... @VaiTon did too much change and added lot of content on his branch for me to handle it, I think it would be better if he can have a look on his side, my branch is essencially about refactoring, I didn't add any special behavior So unless he take a look on it I think my change will be lost here :') I will probably remove some of his work if I try to do it on my side |
teolemon
added
refactor
dependencies
What
Updated the project architecture, how the package is exported / constructed and his dependencies
Fixes bug(s)