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

Cannot find module 'conventional-changelog-*' #589

Closed
1aron opened this issue Jan 26, 2024 · 8 comments
Closed

Cannot find module 'conventional-changelog-*' #589

1aron opened this issue Jan 26, 2024 · 8 comments

Comments

@1aron
Copy link

1aron commented Jan 26, 2024

Everything worked fine before v11.1; since @sheerlox introduces the import-from-esm #537, and it's behavior became unreliable.

To reproduce

This error occurs in both cases.

Case 1

Clone my repo and run the test

git clone https://github.com/1aron/techor.git
cd techor
pnpm i
pnpm build
cd packages/semantic-release-config
pnpm test
> semantic-release-config-techor@ test /Users/aron/techor/packages/semantic-release-config
> jest

 FAIL  tests/rules.test.ts
  ● Parse with "conventional-changelog-techor" by default

    Cannot find module 'conventional-changelog-techor'

      11 |         'Feat(Scope2): Second feature'
      12 |     )
    > 13 |     const releaseType = await analyzeCommits(
         |                         ^
      14 |         { preset: 'techor', releaseRules },
      15 |         { cwd: process.cwd(), commits, logger: console }
      16 |     )

      at importFrom (../../node_modules/.pnpm/import-from-esm@1.3.3/node_modules/import-from-esm/index.js:100:17)
      at async _default (../../node_modules/.pnpm/@semantic-release+commit-analyzer@11.1.0_semantic-release@23.0.0/node_modules/@semantic-release/commit-analyzer/lib/load-parser-config.js:25:63)
      at async analyzeCommits (../../node_modules/.pnpm/@semantic-release+commit-analyzer@11.1.0_semantic-release@23.0.0/node_modules/@semantic-release/commit-analyzer/index.js:31:18)
      at async Object.<anonymous> (tests/rules.test.ts:13:25)

The reproductions above are all in the same monorepo, I think this is not entirely a bug caused by the monorepo, but caused by import-from being changed to import-from-esm.

conventional-changelog-techor is the packages/conventional-changelog-config in the monorepo. This error is not caused by its own error, even if you use conventional-changelog-angular instead and specify preset 'angular'.

The current version has not been changed to asynchronous function configuration, but this is not relevant.

Case 2

It's about my contribution #588. Perform the following steps on latest master branch:

npm i conventional-changelog-techor@2.5.24 -D

Add the new test/presets.test.js

import test from "ava";
import { stub } from "sinon";
import { analyzeCommits } from "../index.js";

const cwd = process.cwd();

test.beforeEach((t) => {
  const log = stub();
  t.context.log = log;
  t.context.logger = { log };
});

test('Accept "preset" option', async (t) => {
  const commits = [
    { hash: "123", message: "Fix: First fix (fixes #123)" },
    { hash: "456", message: "Update: Second feature (fixes #456)" },
  ];
  const releaseType = await analyzeCommits({ preset: "techor" }, { cwd, commits, logger: t.context.logger });

  t.is(releaseType, null);
});
npm test
> Cannot find module 'conventional-changelog-techor'

If I remove the "exports": { ... }" section from "node_modules/conventional-changelog-techor/package.json", it will work.

So this is a problem with import-from-esm itself, regardless of whether it is a monorepo or not.

When I change import-from-esm back to import-from everything still works. This means that the changes in v11.1 did not include tests for the new functionality (#537), and these tests would have failed before v11.1. Changes add a lot of uncertainty.

Suggestion

Reverts:

Potentially relevant

@travi
Copy link
Member

travi commented Jan 26, 2024

The reproductions above are all in the same monorepo, I think this is not entirely a bug caused by the monorepo

in order for us to give any priority to this in the context of semantic-release, we need to eliminate the context of the monorepo from the equation. the use of import-from-esm has proven effective for our known and supported use cases. i'm not trying to suggest that you have not found a problem, but we need to narrow the scope to ensure we can even attempt to capture any potential additional need.

if you want us to investigate this further in the context of the semantic-release project, we need you to provide a public minimal reproduction that does not include the monorepo structure. if it can not be reproduced in that context, this issue needs to be addressed outside of the semantic-release project since we do not officially support monorepos. that may mean that that you need to move this issue either to import-from-esm directly or the monorepo wrapper for semantic-release that you happen to be using. it will then be up to those maintainers to decide how to prioritize the issue.

@1aron
Copy link
Author

1aron commented Jan 26, 2024

The reproductions above are all in the same monorepo, I think this is not entirely a bug caused by the monorepo

in order for us to give any priority to this in the context of semantic-release, we need to eliminate the context of the monorepo from the equation. the use of import-from-esm has proven effective for our known and supported use cases. i'm not trying to suggest that you have not found a problem, but we need to narrow the scope to ensure we can even attempt to capture any potential additional need.

if you want us to investigate this further in the context of the semantic-release project, we need you to provide a public minimal reproduction that does not include the monorepo structure. if it can not be reproduced in that context, this issue needs to be addressed outside of the semantic-release project since we do not officially support monorepos. that may mean that that you need to move this issue either to import-from-esm directly or the monorepo wrapper for semantic-release that you happen to be using. it will then be up to those maintainers to decide how to prioritize the issue.

@travi Got it. The following reproduction is based on the latest commit-analyzer branch.

MVP

git clone https://github.com/1aron/commit-analyzer
git checkout issue-import-from-esm
npm i
npm test
  load-parser-config › undefined Load "techor" config

  Rejected promise returned by test. Reason:

  Error {
    code: 'MODULE_NOT_FOUND',
    message: 'Cannot find module \'conventional-changelog-techor\'',
  }

  Error: Cannot find module 'conventional-changelog-techor'
      at importFrom (file:///Users/aron/commit-analyzer/node_modules/import-from-esm/index.js:100:17)
      at async default (file:///Users/aron/commit-analyzer/lib/load-parser-config.js:28:76)
      at async loadConfig (file:///Users/aron/commit-analyzer/test/load-parser-config.test.js:32:6)



  1 test failed

If you remove the "exports": { ... }" section from node_modules/conventional-changelog-techor/package.json, then it will work.

The error is expected, conventional-changelog-techor has not been changed to an asynchronous function.

  load-parser-config › undefined Load "techor" config

  Rejected promise returned by test. Reason:

  TypeError {
    message: '((intermediate value) || (intermediate value)) is not a function',
  }

  TypeError: ((intermediate value) || (intermediate value)) is not a function
      at default (file:///Users/aron/commit-analyzer/lib/load-parser-config.js:28:107)
      at async loadConfig (file:///Users/aron/commit-analyzer/test/load-parser-config.test.js:32:6)

  ─

  1 test failed

@travi
Copy link
Member

travi commented Jan 26, 2024

If you remove the "exports": { ... }" section from node_modules/conventional-changelog-techor/package.json, then it will work.

ok, so i think i understand the difference from the known working situations. importing a common-js module or an esm module are supported and known to be working. your package for your preset is dual-mode, exporting both cjs and esm. removing the exports map definition makes it simply an esm-only package rather than dual mode.

is there a reason why you need that specific package to be dual mode rather than esm-only (like the official conventional-changelog presets)? i can't think of a benefit of exporting both from a preset package like this.

if you do think this should be supported, i think this is a specific request for import-from-esm, not semantic-release directly. i'll let you take that up with @sheerlox directly through that repo rather than here.

@1aron
Copy link
Author

1aron commented Jan 26, 2024

@travi The primary motivation for offering both ESM and CJS exports in an open-source package is to ensure compatibility. I believe it's crucial for the package to be importable without issues when using the "exports" configuration with multiple entry points. At the very least, 'import-from' functions smoothly.

I could have simplified conventional-changelog into a single CJS or ESM package, but I believe it's important to address this bug through import-from-esm or revert to using import-from.

In the past, semantic-release supported importing its own configuration in monorepo, such as packages/conventional-changelog-config, but since using import-from-esm will cause import errors, I think this compromise is not worth it. .

@travi
Copy link
Member

travi commented Jan 26, 2024

The general consideration for exporting an open source package to ESM and CJS is for compatibility

dual mode packages can be useful for compatibility, but i struggle to understand why a package for a conventional-changelog preset has a need for that level of compatibility. because of the async function breaking change, your package can also have a breaking change for switching from cjs to esm-only, if you choose to. there should not be situations for that package where you need that capability.

I think this bug must be fixed by import-from-esm, or changed back to import-from

i'm trying to be patient with your request, but please understand that this is an OSS project maintained by volunteer maintainers. we are the ones that need to be convinced which options are worth accepting and maintaining long term. i honestly do not see value in doing additional work to support dual-mode preset packages when we do support both cjs-only and esm-only packages and you could simply choose to publish one or the other.

you are welcome to submit an issue against import-from-esm and make your case there, but at this point, i'm going to say that this is not a feature request that we find valuable enough at the semantic-release project level to justify investing in

@travi travi closed this as completed Jan 26, 2024
@1aron
Copy link
Author

1aron commented Jan 26, 2024

Dual-mode support is of course not necessary, my primary concern lies in ensuring that Node's general import behavior functions as expected.

It appears that the issue indeed stems from 'import-from-esm'. As a temporary solution, I will revert the package to version 11.0 by modifying the package.json file:

{
    "resolutions": {
        "@semantic-release/commit-analyzer": "11.0.0"
    }
}

I genuinely appreciate all the contributions made by open-source contributors to this ecosystem. No offense is intended.

@travi
Copy link
Member

travi commented Jan 26, 2024

Likewise, we appreciate the heads up. Just trying to direct you to where this can be best handled. I can agree with the desire to be consistent, it just isn't something that I think we can justify as a priority for the semantic-release project. @sheerlox might be open to it for import-from-esm, but I'll defer there

@sheerlox
Copy link
Member

@1aron for the first case, I'm guessing the issue is coming from pnpm, as outlined in their FAQ.

@semantic-release/commit-analyzer tries to load packages installed along itself (and not installed by itself, since we obviously can't make every possible preset a dependency of this package). pnpm breaks the support for that, because as explained on their homepage : "pnpm creates a non-flat node_modules by default, so code has no access to arbitrary packages".

If you still feel this is an issue with import-from-esm, feel free to open a separate issue on my repo (ideally with a minimal reproduction example).

Regarding the second case, let's track this in the issue you already opened (sheerlox/import-from-esm#92).

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 a pull request may close this issue.

3 participants