Integration with AJV not working.

[shubham-shrivastava]

• Rollup Plugin Name: commonjs
• Rollup Plugin Version: 17.0.0
• Rollup Version: 2.35.1
• Operating System (or Browser): popOS (ubuntu), Chrome (Version 87.0.4280.88 (Official Build) (64-bit))
• Node Version: 10.20.1
• Link to reproduction (⚠️ read below): https://github.com/shubham-shrivastava/rollup-ajv-test

Expected Behavior

There should not be any errors of undefined dependencies in the bundle when using AJV JSON schema validator

Actual Behavior

Two require from the same file results in undefined value for later.
for e.g.
const a = require("./file");
const b = require("./file");

this makes b undefined, hence any function called on b results in error.
It seems commonjs is not able to resolve the dependency correctly.

Running the file with tsc (or node) works fine.

Additional Information

The minimal repo to reproduce the above error is located at - https://github.com/shubham-shrivastava/rollup-ajv-test
Steps to reproduce -

• npm i
• npm run build:dev

This will open a tab in the browser, where you can see the error in the console.

[josdejong]

I'm hitting the same issue, in my case I get an error:

Uncaught TypeError: dataType_2 is undefined

This error is originating in the file ajv/dist/compile/validate/iterate.js:

"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
exports.schemaKeywords = void 0;
const applicability_1 = require("./applicability");
const dataType_1 = require("./dataType.js");
const defaults_1 = require("./defaults");
const dataType_2 = require("./dataType.js");
// ...

The error seems to be gone when I manually remove const dataType_2 = require("./dataType.js"); and replace the usage of dataType_2 with dataType_1.

Ping @epoberezkin. A workaround would be to change the source code of Ajv such that it doesn't require("./dataType.js") twice I think.

[shubham-shrivastava]

I'm hitting the same issue, in my case I get an error:

Uncaught TypeError: dataType_2 is undefined

This error is originating in the file ajv/dist/compile/validate/iterate.js:

"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
exports.schemaKeywords = void 0;
const applicability_1 = require("./applicability");
const dataType_1 = require("./dataType.js");
const defaults_1 = require("./defaults");
const dataType_2 = require("./dataType.js");
// ...

The error seems to be gone when I manually remove const dataType_2 = require("./dataType.js"); and replace the usage of dataType_2 with dataType_1.

Ping @epoberezkin. A workaround would be to change the source code of Ajv such that it doesn't require("./dataType.js") twice I think.

Indeed. it's the same issue I am having.
and it comes from the compiled version of /node_modules/ajv/lib/compile/validate/iterate.ts

import type {SchemaObjCxt} from ".."
import type {JSONType, Rule, RuleGroup} from "../rules"
import {shouldUseGroup, shouldUseRule} from "./applicability"
import {checkDataType} from "./dataType"
import {assignDefaults} from "./defaults"
import {reportTypeError} from "./dataType"

thanks for the workaround, but changing the source code is not feasible, unfortunately (or if every developer can change the source code on their machine in the node_modules, which is not feasible)

[josdejong]

Yes, I mean it would be nice if @epoberezkin would be able to change the source code of Ajv such that it doesn't hit this issue with @rollup/plugin-commonjs (even though this is not really a bug in Ajv itself).

[epoberezkin]

Unfortunately I can't enable no-duplicate-imports with eslint-plugin 3.* and I am getting a ton of errors when I try to switch to 4.*

Obviously, the real problem is that this plugin is not supporting what it should probably support - there may be the cases when multiple requires of the same dependency are needed...

[shellscape]

@epoberezkin if there are specific cases where that antipattern is real and necessary, I'd encourage you to open a modification ticket and enumerate those cases. I can't think of a single valid case, but that's not to say there aren't. I don't believe the plug-in is behaving wrongly here.

[epoberezkin]

It's an opinion ;)

What is important here that it is incompatible with node.js - a call to require returns the export of the module regardless how many times it is called, but the module code should be executed only once (I've never read the spec itself, but node.js is what defines the standard de-facto).

The use case where it is needed is the dependency that is expensive to initialize is loaded on demand inside several functions that need it. The possible alternative is to memoize it in some module scoped variable, but it arguably makes it less obvious what is going on - so it is a matter of opinion which pattern is better.

Not the problem I need solved, though, just some comment in passing. I will publish Ajv change soon.

Happy New Year all :)

[shellscape]

@guybedford and @LarsDenBakker have put a lot of effort into getting commonjs and node-resolve spec compliant with exports - they may disagree with you on that one.

[josdejong]

It's a tricky case. Normally there isn't really a reason to have duplicate imports though it may happen and is valid code. It is just very unfortunate when this gives issues downstream (like now) which are not so easy to detect early (and maybe you're not in control of changing it). Having a linter in place would help prevent it from occurring, but still, it would be great if @rollup/plugin-commonjs would be robust against it 😄. Thanks for all your efforts @guybedford and @LarsDenBakker, this is really complex stuff.

[guybedford]

Yes this is no doubt a bug, I've updated the labels. The fix might not even be too hard if someone wants to look into it. This is a perfect example of the sort of open source bug where picking up a little more ownership on the level beneath you and solving the small issue that directly helps you while also improving the situation for others can, as a general rule, form a healthy ecosystem.

[stale]

Hey folks. This issue hasn't received any traction for 60 days, so we're going to close this for housekeeping. If this is still an ongoing issue, please do consider contributing a Pull Request to resolve it. Further discussion is always welcome even with the issue closed. If anything actionable is posted in the comments, we'll consider reopening it. ⓘ