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

fix: 🐛 use ECMAScript syntax to import ajv #19

Merged

Conversation

johngeorgewright
Copy link
Collaborator

Fixes: #17

Copy link
Owner

@ForbesLindesay ForbesLindesay left a comment

Choose a reason for hiding this comment

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

This will break unless the esModuleInterop option in tsconfig is true, and is only needed if "module": "es2015" is specified. It needs to test the tsconfig that gets loaded and then choose between the two of them. Probably we should use the es6 style if esModuleInterop is true.

@johngeorgewright
Copy link
Collaborator Author

This change seems to work in all "module" or "esModuleInterop" scenarios, however, changing it back will break in "module": "es2015" | "esnext". Is there any reason to switch the different implementations dependent on the settings?

@johngeorgewright
Copy link
Collaborator Author

I've added some tests that attempt to build the generated files with different module settings and all seem to pass. If, however, I revert back to using the import Ajv = require('ajv') syntax these tests fail when using ES2015 and ESNEXT module settings.

@ForbesLindesay
Copy link
Owner

You don't see to have tested the esModuleInterop being false, which is the critical case.

});

const buildProject = async (project: string) => {
await exec(`cp tsconfig.${project}.json tsconfig.json`, {cwd: testDir});
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One problem I noticed is that typescript-json-validator will pull from the project's tsconfig.json file, even if the project was run with a different config file, IE, tsconfig.test.json.

Copy link
Owner

@ForbesLindesay ForbesLindesay left a comment

Choose a reason for hiding this comment

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

Thanks for all the work on this. The tests now look really thorough which is excellent. I think it's nearly there.

src/template.ts Outdated
export const IMPORT_AJV = `import Ajv = require('ajv');`;

export const IMPORT_AJV = (): string => {
const tsConfig = loadTsConfig();
Copy link
Owner

Choose a reason for hiding this comment

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

Can we pass tsconfig in as a parameter. It already gets loaded once in order to generate the json schema. We don't really want to load it again. That way you also won't need to default to process.cwd() and it would be easier to add a --project flag to explicitly select a tsconfig file.

@ForbesLindesay ForbesLindesay merged commit 9ea471f into ForbesLindesay:master Jun 19, 2019
@ForbesLindesay
Copy link
Owner

🎉 This PR is included in version 2.2.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@johngeorgewright johngeorgewright deleted the fix/17-import-syntax branch June 20, 2019 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Import Ajv with ECMAScript syntax
2 participants