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.

Sign up for GitHub

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

Jump to bottom

fix: remove esModuleInterop from tsconfig (#110) #111

Merged
merged 1 commit into from Aug 31, 2020
Merged

fix: remove esModuleInterop from tsconfig (#110) #111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 3 additions & 4 deletions declarations/validate.d.ts
View file
Open in desktop
Expand Up @@ -2,7 +2,7 @@ export default validate;
export type JSONSchema4 = import('json-schema').JSONSchema4;
export type JSONSchema6 = import('json-schema').JSONSchema6;
export type JSONSchema7 = import('json-schema').JSONSchema7;
export type ErrorObject = Ajv.ErrorObject;
export type ErrorObject = import('ajv').ErrorObject;
export type Extend = {
formatMinimum?: number | undefined;
formatMaximum?: number | undefined;
Expand All @@ -13,8 +13,8 @@ export type Schema =
| (import('json-schema').JSONSchema4 & Extend)
| (import('json-schema').JSONSchema6 & Extend)
| (import('json-schema').JSONSchema7 & Extend);
export type SchemaUtilErrorObject = Ajv.ErrorObject & {
children?: Ajv.ErrorObject[] | undefined;
export type SchemaUtilErrorObject = import('ajv').ErrorObject & {
children?: import('ajv').ErrorObject[] | undefined;
};
export type PostFormatter = (
formattedError: string,
Expand All @@ -40,5 +40,4 @@ declare namespace validate {
export { ValidationError };
export { ValidationError as ValidateError };
}
import Ajv from 'ajv';
import ValidationError from './ValidationError';
7 changes: 4 additions & 3 deletions src/validate.js
View file
Open in desktop
@@ -1,10 +1,11 @@
import Ajv from 'ajv';
import ajvKeywords from 'ajv-keywords';

import addAbsolutePathKeyword from './keywords/absolutePath';

import ValidationError from './ValidationError';

// Use CommonJS require for ajv libs so TypeScript consumers aren't locked into esModuleInterop (see #110).
const Ajv = require('ajv');
const ajvKeywords = require('ajv-keywords');
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

@ozyman42 ozyman42 Aug 29, 2020 •
edited

Choose a reason for hiding this comment

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

The way TypeScript implements module imports is by following the ECMAScript module standard. In contrast, Node.js uses CommonJS, with exports and module.exports. Since TypeScript is supposed to work for both Node and Browser (any JS runtime), it should by default follow the language standard of ESModules.
For cases where the author wants to use the import syntax where the library being imported exports via CommonJS, TypeScript offers the esModuleInterop flag, to tell the compiler to treat module.exports = as an ES modules export default.

This gets to the root of the issue. The ajv author is not adhering to the JS language's now-official ES Modules standards. See here and here, so in order to solve the issue I opened (#110), we have two possible approaches:

  1. Change the schema-utils package so it no longer uses esModuleInterop in its tsconfig, so we can ensure that any typescript definitions generated via npm run build:types will result in definitions which don't require direct or transitive consumers to also enable this esModuleInterop flag
  2. Convince the ajv author to make a breaking change so that modules are exported using the ES Modules standard.

So in order to solve the issue without any breaking changes to any package, I've made the change you commented on: replacing the ambiguous import with require. Require has and will always be a CommonJS thing sharing no overlap with the ES Modules spec, so TypeScript can immediately interpret require as a CommonJS import without the need for any esModuleInterop flag.

If I didn't make this change to require, while still trying to remove the esModuleInterop flag, then this repo's npm run build:types would have failed.

Copy link
Member

Choose a reason for hiding this comment

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

@alexleung let's add comments about it to code to avoid misleading for other developers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment above those two require lines.


/** @typedef {import("json-schema").JSONSchema4} JSONSchema4 */
/** @typedef {import("json-schema").JSONSchema6} JSONSchema6 */
/** @typedef {import("json-schema").JSONSchema7} JSONSchema7 */
Expand Down
1 change: 0 additions & 1 deletion tsconfig.json
View file
Open in desktop
Expand Up @@ -6,7 +6,6 @@
"checkJs": true,
"strict": true,
"types": ["node"],
"esModuleInterop": true,
"resolveJsonModule": true
},
"include": ["./types/**/*", "./src/**/*"]
Expand Down