-
Notifications
You must be signed in to change notification settings - Fork 427
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(nango.yaml): dedicated package, stricter parsing, wider types support #2303
Conversation
return true; | ||
} | ||
|
||
parseFields({ fields, parent }: { fields: NangoYamlModelFields; parent: string }): NangoModelField[] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the main function to parse models and handle all the edge cases regarding types
@@ -0,0 +1,154 @@ | |||
import { expect, describe, it } from 'vitest'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved from shared
return !regQuote.test(name); | ||
} | ||
|
||
export function getProviderConfigurationFromPath({ filePath, parsed }: { filePath: string; parsed: NangoYamlParsed }): NangoYamlParsedIntegration | null { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved from shared
.map((v) => v.toLocaleLowerCase()); | ||
const typesWithGenerics = [...JAVASCRIPT_AND_TYPESCRIPT_TYPES.builtInObjects, ...JAVASCRIPT_AND_TYPESCRIPT_TYPES.utilityTypes]; | ||
// Only used externally | ||
export function isJsOrTsType(type?: string): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved it before realizing it wasn't what I expected, it will be deleted as soon as I migrate the backend to use the new schema
* and then 1636 etc. The offset should be based on the interval and should never be | ||
* greater than the interval | ||
*/ | ||
export function getInterval(runs: string, date: Date): IntervalResponse | Error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved from shared, with the diff that ServiceResponse does not exists and not available in CLI so I just return an Error if any
offset: number; | ||
} | ||
|
||
export function determineVersion(configData: NangoYaml): 'v1' | 'v2' { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved from shared
"copy:files": "copyfiles -u 1 lib/templates/* dist", | ||
"copy:types": "cp `node -p \"require.resolve('@nangohq/shared/dist/sdk/sync.d.ts')\"` './dist/nango-sync.d.ts'", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one is a big bet, I'm trying to load the file from user's global node_modules, should work but let's see :D
expect(modelsFile).not.toContain(`'Other[]' | null | undefined;`); | ||
expect(modelsFile).toContain(`Other[] | null | undefined;`); | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of this has been moved either to model.service.ts or removed because no longer relevant
This seems to happen when there is a
with this configuration gives:
If a model is reference but doesn't actually exist It seems the
produces:
where previously it would correctly output:
Given this:
The outputted model is
which is unexpected. I see why you did it to encourage people to use models but IMO an input of a string is perfectly valid and shouldn't require a model. But maybe this is more of a product question and good for Bastien to weigh in. Also if a model is declared but doesn't actually exist the yaml parsing continues but feel like we should catch this? |
When running
now just gives
|
Can you give examples of each of these?
|
✅ Thanks, missed that in my tests
✅ Okay found the issue thanks
Added the models output, but yeah I'm not keeping the raw format in memory
It's still perfectly valid, but I need one model per input/output so that I can generate json schema (and more) in a single way. With just one way there is no need to check everywhere in the backend if a string is jsType or a model.
That's the problem of supporting js types, there is no way to differentiate literal string and model name. So I added a warning about literal string but I don't see a better way rn
You have in the test and PR desc. models:
Ref:
id: string
Ref2:
name: string
Base:
unions: string | null | Ref | true[]
arrUnion:
- Ref
- true
literal: hello
num: 1
literalNull: null[]
obj:
nested: string
optional?: string
__extends: Ref,Ref2 |
import { getNativeDataType, getPotentialTypeAlias, isDisallowedType, shouldQuote } from './helpers.js'; | ||
import { ParserError, ParserErrorCycle, ParserErrorDataSyntax, ParserErrorExtendsNotFound, ParserErrorInvalidModelName } from './errors.js'; | ||
|
||
export class ModelsParser { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nit but I would keep this singular, ModelParser
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean, it's like that because it's parsing the models
list and contains all the models maybe ModelsPropParser would have been clearer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nits, but works well. I was using this branch as I was writing customer scripts today 💯
This is a bit weird:
{
"name": "selectedFields",
"value": [
{
"name": "0",
"value": "SelectedField[]",
"model": true,
"optional": false
},
{
"name": "1",
"tsType": true,
"array": false,
"optional": false
},
{
"name": "2",
"value": null,
"tsType": true,
"array": false,
"optional": false
}
],
"union": true,
"optional": false
}
This is when the type is like this
selectedFields: SelectedField[] | undefined | null
weird in the sense that the name is the array index, but get that this might be technically challenging.
Interesting, actually it should be supported. Adding this edge case, thanks a lot for the thorough testing 🙏🏻 |
Fixes NAN-1166
Fixes https://linear.app/nango/issue/NAN-1096/add-comment-at-the-top-of-modelsts-file-to-explain-that-its-auto
🤓 Context
First I want to apologize for the length of this PR, it's way too big but it was "all or nothing" situation unfortunately.
When I started playing with types/json schema validation I wanted better output for errors at compile time:
So I had no choice, to rewrite everything just because I wanted to cover more cases to be compatible with json schema 😵💫
The Good news:
There is only 13 mentions to shared left in CLI
🚜 Changes
New published package
@nangohq/nango-yaml
It will be published, as soon as you validate this PR otherwise the CLI won't pass.
New output when validating nango.yaml
A lot of new edge cases, errors and warnings are now discovered. It might be breaking but at least it's explicit.
[breaking]
endpoint
format is enforced (verb /uri)[breaking]
runs
is explicitly required[breaking] format of
NangoFlows
inside model.ts[breaking] Input and Output specifying a js value (instead of a model) now use a
type
interface with a unique generated name[new] Support for unions, arrays, type array, cyclic ref, literal string, number, true, false, null, undefined, multiple extends
[fix] Disallow exotic char in model and property name
[fix] Always compile model.ts after loading nango.yaml
...not exhaustive changelist
Warning
UI won't show the appropriate model (fixing that later) and this is a breaking change for
GET /flows
GET /config/:key
😭 How do I review this huge PR?
packages/nango-yaml/parser.v2.ts
packages/nango-yaml/modelsParser.ts
which handle the yaml -> js typespackages/cli/services/model.service.ts
packages/types/nangoYaml/index.ts
(describe yaml then parsed yaml)🧪 How do I test?
Please run the cli on your machine, I have extensively tested but it's so critical and super user dependent.
Example