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

Load JS modules and JSON using require(). #344

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

elyobo
Copy link

@elyobo elyobo commented May 28, 2019

Fixes #343.

Minimal tests are also provided for JS module loading, but there don't currently seem to be tests for stdin processing.

One major change here that could be avoided is that JSON is also directly loaded via require() instead of handing over the content to the yaml parser. This can be avoided if desirable.

@elyobo elyobo marked this pull request as ready for review May 28, 2019 23:12
@elyobo
Copy link
Author

elyobo commented May 31, 2019

Note that this is possibly surplus to requirements (although maybe useful anyway?) given that you can just convert your module in to JSON and pipe it to speccy anyway, e.g.

node -e 'console.log(JSON.stringify(require("./path/to/api.js")))' | npx speccy lint

Feel free to close if not useful, converting to JSON is a reasonable workaround.

@elyobo
Copy link
Author

elyobo commented Jun 7, 2019

I think that for other purposes being able to load JS modules would still be useful, e.g. I've just written a test runner for Jest which uses oas-linter; speccy's loader behaviour would be better if I could hook in to that instead, but my personal use case is JS modules so breaking that is undesirable.

@djtarazona
Copy link
Contributor

Any thoughts on this one @pderaaij?

@elyobo
Copy link
Author

elyobo commented Jun 17, 2019

@djtarazona Rebased in latest changes from master, resolving conflicts.

@pderaaij
Copy link
Contributor

I'm not sure if there is a need for sharing API specifications in JS files. But I see no harm nevertheless. I am wondering if the format flag is required as it make things more verbose and complicated.Perhaps we can simplify this.

@elyobo could you have a look at it or else I can have a look, but that will be a bit later this week.

@elyobo
Copy link
Author

elyobo commented Jun 20, 2019

Thanks @pderaaij.

The use case isn't so much sharing them as JS files, but using JS files to generate them (composition and comments are handy). Ultimately these will be converted to a more shareable format (e.g. node -e 'console.log(JSON.stringify(require("./path/to/api.js")))' | npx speccy lint as shown), but it's certainly nicer not to have to.

The --format flag is only required for standard input, as otherwise the input format is unclear (it's surprising to me that the yaml parser also parses JSON 😮 but handy as it allows you to avoid this flag so far). I could try to infer the format from the file content instead, or I could simply drop the flag and not accept JS on standard input - I added it for consistent support with the other formats, but if that's not an issue then removing it entirely makes the PR very simple.

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.

Feature: Support JS modules
3 participants