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

feat: add jest transformer support #6

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

Conversation

theBenForce
Copy link

Adds support for jest transformers so that a YAML transformer can be added to the jest config without creating a .js file for every api definition.

@theBenForce theBenForce requested a review from elyobo January 14, 2020 19:41
Copy link
Owner

@elyobo elyobo left a comment

Choose a reason for hiding this comment

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

Thanks @theBenForce, this looks like a really useful addition. I've made a few suggestions, most of which are minor, but one major thing I'd like to look at is making use of the @jest/transform functionality to support transforms more correctly and efficiently (and I'm not entirely sure how to go about doing that).

@@ -30,7 +33,7 @@ This is very alpha; no tests have been written, no promises made, YMMV.

## Compatibility

This module is compatible with Node v8.x and upwards. Any incompatibilities with those versions should be reported as bugs.
This module is compatible with Node v8.x and upwards. Any incompatibilities with those versions should be reported as bugs.
Copy link
Owner

Choose a reason for hiding this comment

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

No need for copy editing :) I still haven't kicked the two-spaces-after-period habit that I was taught when younger.

Comment on lines +19 to +27
module.exports = {
runner: "oas-linter",
displayName: "oas-linter",
transform: {
"\\.yaml$": "jest-yaml-transform-not-flattened"
},
testMatch: ["<rootDir>/api/*.yaml"],
moduleFileExtensions: ["yaml"]
};
Copy link
Owner

Choose a reason for hiding this comment

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

Minor, but I prefer ' for strings and trailing commas please.

"<rootDir>/path/to/another/api/doc.js",
"<rootDir>/path/to/another/api/doc/**/*.js"
]
};
Copy link
Owner

Choose a reason for hiding this comment

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

No need to change these, leave as single quote and with trailing comma.

runner: 'oas-linter',
testMatch: ['<rootDir>/path/to/your/api/doc.js'],
runner: "oas-linter",
testMatch: ["<rootDir>/path/to/your/api/doc.js"]
Copy link
Owner

Choose a reason for hiding this comment

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

No need to change these, leave as single quote and with trailing comma.

* `loadDefaultRules` is boolean and defaults to true, [oas-linter default rules](https://github.com/Mermade/oas-kit/blob/master/packages/oas-linter/rules.yaml) can be seen over there
* `rules` is an array of objects, as per oas-linter's [applyRules](https://github.com/Mermade/oas-kit/blob/master/packages/oas-linter/index.js#L12). More details about the format of rules supported can be found over at `oas-kit`'s [linter rules](https://mermade.github.io/oas-kit/linter-rules.html) documentation and the default rules (link above) have many examples.
- `loadDefaultRules` is boolean and defaults to true, [oas-linter default rules](https://github.com/Mermade/oas-kit/blob/master/packages/oas-linter/rules.yaml) can be seen over there
- `rules` is an array of objects, as per oas-linter's [applyRules](https://github.com/Mermade/oas-kit/blob/master/packages/oas-linter/index.js#L12). More details about the format of rules supported can be found over at `oas-kit`'s [linter rules](https://mermade.github.io/oas-kit/linter-rules.html) documentation and the default rules (link above) have many examples.
Copy link
Owner

Choose a reason for hiding this comment

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

No need for changes here.

const schema = require(testPath)
const { transform } = config

let schema
Copy link
Owner

Choose a reason for hiding this comment

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

No need for let here, just use const down where it's defined.

@@ -123,7 +139,7 @@ const run = ({ testPath, config, globalConfig }) => {
return resolve(
fail({
...result,
errorMessage: 'Not a valid OpenAPI schema.'
errorMessage: 'Not a valider OpenAPI schema: ' + error.message
Copy link
Owner

Choose a reason for hiding this comment

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

Use a template string.

Suggested change
errorMessage: 'Not a valider OpenAPI schema: ' + error.message
errorMessage: `Not a valider OpenAPI schema: ${error.message}`,

.map(x => new RegExp(x))
.find(x => x.test(testPath))
) {
const transformer = transform.find(x => new RegExp(x[0]).test(testPath))
Copy link
Owner

Choose a reason for hiding this comment

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

transform is not an array in the config; is this transformed by jest before the runner is called? It looks like it might have been based on your use of x[0] here for the key and x[1] below for creating the transform object; if not you might want to use Object.entries(transform).find() instead.

const transformer = transform.find(x => new RegExp(x[0]).test(testPath))

if (transformer) {
const transformerObj = require(transformer[1])
Copy link
Owner

Choose a reason for hiding this comment

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

It looks like transforms can be provided as an array as well, if we're going to support the jest config we should deal with that too.


// Avoid caching for `--watch` mode
delete delete require.cache[require.resolve(testPath)]
schema = nodeEval(fileContent, testPath)
Copy link
Owner

Choose a reason for hiding this comment

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

I feel like we should be using the ScriptTransformer from @jest/transform which seems to deal with the ignore paths, permutations of how it can be configured, caching and joining of regexes, caching of transformed source. Unfortunately it's really unclear to me how we do that (or even if we need to; if we specify the transforms does jest already handle this for us or do we need to explicitly handle it like this in custom runners?)

@SimenB is there any documentation available on how to support transforms inside custom runners available? It might be a useful addition to the docs for https://github.com/jest-community/create-jest-runner.

Copy link

@SimenB SimenB Jan 15, 2020

Choose a reason for hiding this comment

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

It's up the to runner to run transforms, so you'll have to implement it here

I don't think we have any docs for it - I haven't really thought about this use case. This is untested, but should work

const { readFileSync } = require("fs");
const { ScriptTransformer } = require("@jest/transform");

const run = ({ testPath, config, globalConfig }) => {
  const content = readFileSync(testPath, "utf8");

  const scriptTransformer = new ScriptTransformer(config);

  const transformedContent =
    scriptTransformer.transformSource(testPath, content, false).code;

  // use/eval transformedContent however you'd like
};

Happy to take a PR documenting this 👍

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the comments @elyobo, I think most of the issues come from vscode formatting the files on save. Do you have an editor config or .prettierrc that I could use to format everything properly?

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.

3 participants