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

make it possible to passthrough functions to the swagger-initializer.js #37

Merged
merged 13 commits into from
Feb 17, 2023

Conversation

Uzlopak
Copy link
Collaborator

@Uzlopak Uzlopak commented Feb 15, 2023

Closes #25

I know that the coverage is failing, but writing 100% test coverage would be a waste of time if the approach would not be accepted.

So currently we JSON.stringify uiConfig and initOAuth which is basically removing functions as functions are not per definition serializable. But with my super simple serializiation function, I can serialize an Object as best as possible. Yeah sure... ES6 Maps and Sets and what not are not supported or yet. But whatever. Supplying the initOAuth and uiConfig per swagger-initialization.js so we can support functions.

Removes the initOAuth and the uiConfig routes as they dont need to be fetched again.

Checklist

lib/routes.js Outdated Show resolved Hide resolved
@Fdawgs
Copy link
Member

Fdawgs commented Feb 15, 2023

Docs would need to be updated to mention no support for Maps, Sets etc. until they're added, right?

test/route.test.js Outdated Show resolved Hide resolved
@Uzlopak
Copy link
Collaborator Author

Uzlopak commented Feb 15, 2023

Ok, seems like you have interest in this solution. Added Maps and Sets. but tbh... they werend supported till now neither.

I think the solution is complete.

README.md Outdated Show resolved Hide resolved
Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let wait a bit for the others to review before merging.

README.md Outdated Show resolved Hide resolved
Co-authored-by: Frazer Smith <frazer.dev@outlook.com>
Copy link
Member

@Fdawgs Fdawgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, as Climba said, need some more eyes on it.

Is the CI linting failure an issue?

@climba03003
Copy link
Member

Is the CI linting failure an issue?

Seems like a bug of standard.
It should not crash itself as a linting tools.

@Uzlopak
Copy link
Collaborator Author

Uzlopak commented Feb 16, 2023

I investigated why suddenly linting is failing, and I have no clue. Locally it works. If I remove the standard rules in package.json i get the same error as in the ci.

@Uzlopak
Copy link
Collaborator Author

Uzlopak commented Feb 16, 2023

The linting issue can be found here estools/esquery#135
Not related to our code

@Uzlopak
Copy link
Collaborator Author

Uzlopak commented Feb 16, 2023

Ok. Linting is working now after equery was fixed

lib/routes.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Uzlopak and others added 2 commits February 16, 2023 15:54
Co-authored-by: Manuel Spigolon <behemoth89@gmail.com>
@Uzlopak
Copy link
Collaborator Author

Uzlopak commented Feb 16, 2023

@mcollina
Should the serializer be a separate npm package?

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina
Copy link
Member

No I don't think it's needed at this time.

@Uzlopak
Copy link
Collaborator Author

Uzlopak commented Feb 17, 2023

@Eomm PTAL

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina merged commit e2daa82 into master Feb 17, 2023
@Uzlopak Uzlopak deleted the functions-to-configure branch February 17, 2023 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

uiConfig cant handle functions
5 participants