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

feat: ability to decorate esm module name before importing it #4945

Merged
merged 3 commits into from
Dec 4, 2022
Merged

feat: ability to decorate esm module name before importing it #4945

merged 3 commits into from
Dec 4, 2022

Conversation

j0tunn
Copy link
Contributor

@j0tunn j0tunn commented Nov 11, 2022

Description of the Change

This PR adds ability to decorate ESM-module name right before it will be imported by passing esmDecorator function to loadFilesAsync method.

Why should this be in core?

It helps to deal with ESM cashing issues just by adding some query parameters to modue name:

mocha.loadFilesAsync({
    esmDecorator: file => `${file}?foo=bar`
})

And it backwards compatible.
Note though that the sub-dependencies of the test files will not be reloaded.

Applicable issues

It is an enhancement (minor release).

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 11, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: j0tunn / name: Anton Usmansky (93082f2)

@juergba
Copy link
Contributor

juergba commented Nov 12, 2022

@giltayar could you have a look a this PR, please?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 94.328% when pulling 93082f2 on j0tunn:master into 8f3c37b on mochajs:master.

Copy link
Contributor

@giltayar giltayar left a comment

Choose a reason for hiding this comment

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

Besides the comment I added, I am not sure why we need this. What is the use case behind it? Who will use this additional parameter and for what use case.

Until we understand this, I'm not sure we should merge this PR.

lib/mocha.js Show resolved Hide resolved
@j0tunn
Copy link
Contributor Author

j0tunn commented Nov 22, 2022

Who will use this additional parameter and for what use case.

Any who has problems with esm modules caching while using mocha by API:

node.js doc suggests only one whay to deal with it - different queries.

There are not too many such cases for now because most of the existing community uses CJS modules for tests for now. But there will be more.
You have a bunch of code for dealing with CJS modules cache, but nothing for ESM-modules. And while this cache problem is not solved you can't say you have full support of ESM modules.

@giltayar
Copy link
Contributor

@j0tunn Ah. I didn't know that Mocha.prototype.loadFileAsync was used as an API. In that case it makes total sense given that decorating the urls is the only way to load a different instance of the test files.

Note though that the sub-dependencies of the test files will not be loaded. You will probably need an ESM loader for that.

So if you change the small change that I requested, and add appropriate tests, this PR will be approved by me. But don't forget those tests...

@j0tunn
Copy link
Contributor Author

j0tunn commented Nov 23, 2022

@giltayar done

@giltayar
Copy link
Contributor

@juergba from my point of view, we're good to go on this PR, which makes a lot of sense for developers using the API in an ESM setting.

@juergba juergba added type: feature enhancement proposal semver-minor implementation requires increase of "minor" version number; "features" area: node.js command-line-or-Node.js-specific labels Nov 24, 2022
@juergba
Copy link
Contributor

juergba commented Nov 24, 2022

@giltayar thank you for your review. 👍

I will have a look at this PR on coming weekend, @j0tunn please have some patience.

@juergba
Copy link
Contributor

juergba commented Nov 27, 2022

@j0tunn Mocha has its own --require option which is able to load ESM modules as well. How does it behave with this PR?

Could you provide a short sample for me to do some testing, please?

@fvictorio would you mind reviewing this PR? Does it fix your issue?

@j0tunn
Copy link
Contributor Author

j0tunn commented Nov 28, 2022

@juergba

@j0tunn Mocha has its own --require option which is able to load ESM modules as well. How does it behave with this PR?

As far as I understand --require option is about cli. This PR is about API. These are for different use cases. They are not conflicting if that is your question.

Could you provide a short sample for me to do some testing, please?

There is a short sample in PR description. In case of using mocha by API someone can use this feature like this in its own code to avoid esm-modules cache problem:

mocha.loadFilesAsync({
    esmDecorator: file => `${file}?foo=${new Date()}`
});

@fvictorio
Copy link

Thanks a lot for working on this @j0tunn!

I can confirm this seems to fix my issue if I do something like this:

        await (mocha as any).loadFilesAsync({
          esmDecorator: (file) => `${file}?invalidateCache=${Math.random()}`
        });

Notice the as any: I think this will need a PR in @types/mocha. I can do that, but I'm not familiar with the process. Do you have to wait until this is released before updating the types?

@juergba
Copy link
Contributor

juergba commented Nov 29, 2022

[...] I think this will need a PR in @types/mocha [...]

I don't know, @types/mocha isn't maintained by us. Normally we publish first, then the types get adapted.

I can confirm this seems to fix my issue if I do something like this

I won't merge this PR without any testing, this could take some time though.

Note though that the sub-dependencies of the test files will not be loaded.

@j0tunn could you add this statement to this PR's description, please?

@j0tunn
Copy link
Contributor Author

j0tunn commented Nov 30, 2022

@j0tunn could you add this statement to this PR's description, please?

Done.

@juergba juergba linked an issue Dec 4, 2022 that may be closed by this pull request
@juergba juergba added the run-browser-test run browser tests on forked PR if code is safe label Dec 4, 2022
@github-actions github-actions bot removed the run-browser-test run browser tests on forked PR if code is safe label Dec 4, 2022
Copy link
Contributor

@juergba juergba left a comment

Choose a reason for hiding this comment

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

@j0tunn thank you

This was referenced Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: node.js command-line-or-Node.js-specific semver-minor implementation requires increase of "minor" version number; "features" type: feature enhancement proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

loadFilesAsync and unloading files
5 participants