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!: migrate to ESM #77

Merged
merged 7 commits into from
Nov 3, 2023
Merged

feat!: migrate to ESM #77

merged 7 commits into from
Nov 3, 2023

Conversation

mdonnalley
Copy link
Contributor

@mdonnalley mdonnalley commented Oct 26, 2023

What does this PR do?

Migrate to ESM

What issues does this PR fix or reference?

@W-14374849@

BREAKING CHANGES: ESM and node 18 minimum
@@ -192,7 +192,7 @@
]
},
"test:deprecation-policy": {
"command": "\"./bin/dev\" snapshot:compare",
"command": "ts-node \"./bin/dev.js\" snapshot:compare",
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -74,3 +74,9 @@ export const descriptionTransform = (description: string): string =>
)
// remove empty lines
.replace(/\n{2,}/gm, '\n');

export default {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this needed? Did it not work to just import the functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I've found is that if you intend to stub something it needs to be imported the same way by both the source file and the test file. On top of that, sinon needs to have a module to stub instead of the function itself.

That means that sinon needs import things from 'discoveryQuery, which means that the source file has to do the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

eww. That's going to be dangerous. Put that on your list of "things you need to know about ESM"

Copy link
Contributor

Choose a reason for hiding this comment

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

I also don't like that "mockability for tests" is driving the code structure. Maybe something like esmock could let tests handle this kind of thing?

https://github.com/iambumblehead/esmock/wiki

and also help with all the node:foo stuff?

@mshanemc mshanemc merged commit 267cc84 into main Nov 3, 2023
7 checks passed
@mshanemc mshanemc deleted the mdonnalley/esm branch November 3, 2023 12:54
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.

2 participants