Support extending ESM based configurations

[dominykas]

Closes #3036

I think that's all this takes, right?

• I'm a little uncertain about importing the JSON files - I could fall back to require for it? Node.js docs say that it's in "Active development" and the standard changed from "assertions" to "attributes" in v21... But it works (i.e. tests are passing) now?
• Do I need an integration test? (Haven't looked at what's in there)
• I have not added a test for *.js + "type": "module" in package.json... should I?
• I am by far not an expert in ESM... any edge cases to consider?

[travi]

i started down this path as part of the esm conversion, but backed away from it because of json based configs. i'm not sure that i fully explored that need at the time and, without some further consideration, i'm not even sure i proved that it was a valid use case to account for.

i would want to at least dig deep enough to prove whether that is a valid case since i dont recall the full context right now. if it is, that is the one additional use case i'd want to make sure is considered.

the fact that it wasnt handled at the time of the esm conversion was simply a matter of deferring the change in order to get the rest of the conversion out the door. we always intended for this to be a follow up, so appreciate the help to get this moving, @dominykas (and @bryanjtc)!

[dominykas]

From the linked comment:

import assertions should be available, but would require knowing the format of the config file before loading it

Yeah, that's what I did - I detect JSON based on the extname and add an import assertion. I don't think having a file without an extension used to work with require(), so extension based detection should be OK? And in that case - the only question that I'd have is whether we trust the import assertions - an alternative is to fall back to require or or JSON.parse for JSON?

Did I miss anything? This is all that I saw covered by tests... Happy to add whatever is missing.

[travi]

i think this exercises enough for me to feel good about this change since the imports are actually happening within the scope of these unit tests. we have a couple of cases in our integration test, but i dont think additional tests there would add more regression protection in this case. i cant think of anything outside of what you've handled, so this looks great to me

[travi]

great contribution. thanks a ton for getting us past this detail!

[github-actions]

🎉 This PR is included in version 22.0.7 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[dominykas]

Thanks!

[travi]

@dominykas could this error be related to this change? i forgot to consider the windows case that requires a proper url and we dont have the windows pipeline working for this repo yet to help catch issues like that. since you are most familiar with your change, i'm curious what insight you could provide for this one. thanks!

[travi]

from semantic-release/commit-analyzer#537 (comment):

Since I added JSON module support yesterday, I just tried fixing the issue in semantic-release by using that library, which works well overall, except there's a test that tries to import a node_modules package containing a single index.json file (and no package.json). Currently, the library isn't able to handle that (and it is not specified in the NodeJS docs, although require seems to handle it correctly), but I think it wouldn't even work with an export field pointing to that file in package.json.

Unfortunately, I won't be available in the next few days so I won't be able to work on that issue. Maybe it would be worth prepending file:// in the meantime until I can tackle that issue in import-from-esm?

i'm ok with either approach. if import-from-esm will work well in this situation, i see benefit in the consistency. if that delays things, the alternative approach makes sense to me. we can always follow up to make them consistent as a follow up once getting the situation fixed.

there's a test that tries to import a node_modules package containing a single index.json file (and no package.json). Currently, the library isn't able to handle that (and it is not specified in the NodeJS docs, although require seems to handle it correctly), but I think it wouldn't even work with an export field pointing to that file in package.json.

@sheerlox i could use help understanding this. the only test that i find mentioning an index.json does also provide a package.json. and to clarify, is the complexity the name of the file, the lack of a package.json, or a lack of node_modules?

semantic-release/test/get-config.test.js

Lines 323 to 347 in 02f2cb1

	test.serial('Read configuration from module path in "extends"', async (t) => {
	// Create a git repository, set the current working directory at the root of the repo
	const { cwd } = await gitRepo();
	const pkgOptions = { extends: "shareable" };
	const options = {
	analyzeCommits: { path: "analyzeCommits", param: "analyzeCommits_param" },
	generateNotes: "generateNotes",
	branches: ["test_branch"],
	repositoryUrl: "https://host.null/owner/module.git",
	tagFormat: `v\${version}`,
	plugins: false,
	};
	// Create package.json and shareable.json in repository root
	await outputJson(path.resolve(cwd, "package.json"), { release: pkgOptions });
	await outputJson(path.resolve(cwd, "node_modules/shareable/index.json"), options);
	// Verify the plugins module is called with the plugin options from shareable.json
	td.when(plugins({ cwd, options }, { analyzeCommits: "shareable", generateNotes: "shareable" })).thenResolve(
	pluginsConfig
	);
	const result = await t.context.getConfig({ cwd });
	// Verify the options contains the plugin config from shareable.json
	t.deepEqual(result, { options, plugins: pluginsConfig });
	});

[travi]

from semantic-release/commit-analyzer#537 (comment)

sorry - I do not have Windows to test this myself either.

understood and not a problem. it really comes down to the lack of a windows pipeline for the project, which is on us. we'd started an effort to get that back in place, but i failed to see it through fully. this can be motivation to revisit.

[travi]

I'm on Windows. What needs doing? :)

@seebeen the remaining windows compatibility details would be related to this PR. if you want to test out the current state to confirm the problem and maybe try out the proposed options for fixing, that could be really helpful. we should also work toward getting the windows workflow that i mentioned above into the mix so that we can avoid this sort of situation going forward.

[sheerlox]

@sheerlox i could use help understanding this. the only test that i find mentioning an index.json does also provide a package.json. and to clarify, is the complexity the name of the file, the lack of a package.json, or a lack of node_modules?

Sorry for the delay. Indeed the test I was referring to has a package.json: the complexity is that the package.json does not provide a main field, and also that import-from-esm relies on extension detection to fallback to require when loading JSON files. Since the test tries to load an NPM package, this detection cannot occur and the import fails. I haven't checked if providing a main field would change anything, but I'll get to this first thing tomorrow and follow up here. I have a Windows dual-boot for this kind of issue, so I'll be able to make sure it works as expected.

[sheerlox]

Forgot to mention that I fixed the underlying issue in import-from-esm today and prepared a PR that should fix the issue at hand with semantic-release: #3062

[travi]

@mcmiddle592 the issue you reported in semantic-release/commit-analyzer#537 (comment) should be resolved by https://github.com/semantic-release/semantic-release/releases/tag/v22.0.8 since it was most likely introduced by an oversight in this change. would really appreciate it if you could confirm if this new version resolves the issue for you. thanks again for reporting!

[mcmiddle592]

@travi @sheerlox it is working now. Thanks for all the help!