-
Notifications
You must be signed in to change notification settings - Fork 42
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
Revert: "revert: Revert "chore: introduced typescript support"" #4451
Revert: "revert: Revert "chore: introduced typescript support"" #4451
Conversation
@mzedel, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline". my commands and optionsYou can trigger a pipeline on multiple prs with:
You can start a fast pipeline, disabling full integration tests with:
You can trigger GitHub->GitLab branch sync with:
You can cherry pick to a given branch or branches with:
|
"skipLibCheck": true | ||
}, | ||
"extends": "./tsconfig.json", | ||
"exclude": ["src/js/api/types/Settings.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.
why this file excluded?
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.
It has been a while since I've sat this up but if I remember correctly, the 2fa attribute name is not allowed in TS as it starts with a number and I think that was the whole reason for the workaround with the extra config file...
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.
ah well it seems to be not to be working as I expected after all: microsoft/TypeScript#7432 (comment)
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.
hmm then I will remove the extra file and... adjust the type generation to rewrite the 2fa
name? or exclude the file from the barrel file? or... skip checking these types? all not exciting 😬
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.
now I ended up adjusting the type generation to ignore the include of the Settings.ts
and not even try to resolve the import... So... what do you think about 73dc87c ? tolerable or too risky?
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 think it's not perfect, but acceptable, maybe we should rename 2fa
to twoFA
at some point?
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.
Yeah, that would be good longer term! Unfortunately I think it will break the API spec/ require a version bump just for this - so likely little appetite in the backend 😬... I'll create a ticket 👌
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.
do we need this file?
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.
Quite sure, see the other comment
This reverts commit bbc23a4. Signed-off-by: Manuel Zedel <manuel.zedel@northern.tech>
Signed-off-by: Manuel Zedel <manuel.zedel@northern.tech>
Signed-off-by: Manuel Zedel <manuel.zedel@northern.tech>
439e53c
to
73dc87c
Compare
This reverts commit bbc23a4.