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: remove esModuleInterop from tsconfig (#110) #111

Merged
merged 1 commit into from Aug 31, 2020

Conversation

ozyman42
Copy link
Contributor

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

#110

Breaking Changes

none

Additional Info

allows direct and transitive consumers of this dependency to no longer be required to add esModuleInterop to their tsconfig.json

@codecov
Copy link

codecov bot commented Aug 27, 2020

Codecov Report

Merging #111 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #111   +/-   ##
=======================================
  Coverage   97.57%   97.57%           
=======================================
  Files           6        6           
  Lines         700      702    +2     
  Branches      299      299           
=======================================
+ Hits          683      685    +2     
  Misses         15       15           
  Partials        2        2           
Impacted Files Coverage Δ
src/validate.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a5d216...7b70353. Read the comment docs.

import addAbsolutePathKeyword from './keywords/absolutePath';

import ValidationError from './ValidationError';

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

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.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Thanks

@alexander-akait alexander-akait merged commit 2f40154 into webpack:master Aug 31, 2020
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.

None yet

2 participants