-
Notifications
You must be signed in to change notification settings - Fork 19
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
Enrich the docs when it comes to types #272
Conversation
34e1ac3
to
2c9777f
Compare
2c9777f
to
d765793
Compare
please move the example from the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see suggested changes.
README.md
Outdated
@@ -22,6 +22,8 @@ await esmock( | |||
``` | |||
|
|||
`esmock` examples | |||
|
|||
JavaScript |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the JavaScript and TypeScript labels. Subjectively, I don't they are necesssary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree, otherwise we would have merged the two section together, it's your call tho.
@@ -90,6 +92,21 @@ test('esmock.strict mocks', async () => { | |||
}) | |||
``` | |||
|
|||
TypeScript | |||
``` typescript | |||
import type MinecraftWorld from '../src/minecraftWorld.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Include all of imports needed for the example, and separate the imports from the first test with an empty newline. Remove the dangling comma after 'HARD',
import test from 'node:test'
import assert from 'node:assert'
import esmock from 'esmock'
import type MinecraftWorld from '../src/minecraftWorld.js'
test('allows you to specify the type of returned exports', async () => {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the dangling comma after 'HARD',
This is actually a best practice, see here.
There is a lint plugin that should int the README but |
I don't believe this functionality needs to be documented in the README. The ability to use typescript types is expected from esmock, and therefore it is not really a "feature" that needs to be document or explained. Rather, it is a bug if esmock types are specified incorrectly so as to not allow using import types. The other examples in the README are needed, because they demonstrate features of esmock used for mocking modules in unit tests which is the purpose of esmock. What do you think @koshic @ahmed-hritani? |
@iambumblehead, documentation should describe all 'non-default' behavior aspects. For TS, we expect that any function has some return type (void, undefined, any, doesn't matter). And based on excellent examples, it's easy to understand that return type is 'any', as the same as for the import() statement. What is not default and not expected - esmock function has generic signature and can accept optional type parameter. It's a 'feature' in TS terms, some piece of code in .d.ts file. So, I think it should be documented. Do we need separate example or just 'TS usage' section? Don't know, it's a matter of taste. |
Adding an import README example sounds like a good idea, based on the reasoning above. |
Great job, you just saw somebody's idea, made it tooo hard for them to do it, then copied it and did it yourself, this is the definition of toxic behaviour. Your PR here also shows how "open" this open source repo is. |
@ahmed-hritani if you really want to discuss it (because I strongly disagree) - let's create some thread outside of that PR (or even GitHub). |
Some observations,
Do you always enter new projects telling people what to do? What authority qualifies you as an expert? My own un-solicited advice,
|
Hmm, your "obersvations" are not so accurate, let me show you how: The example should work. It should be possible to write a real unit-test that passes with the example, I think that you could actually use the "advices" that you've added for your own sake, on top of that, you did actually waste my time, when you merge and revert multiple times as you do, when you review the same PR multiple times when you could have done so in one time you waste my time, when you CLOSE my PR that I've invested my time with and COPY my idea and add it with YOUR PR you have both wasted my time and stole it. The sole reason that I've contributed to the repo was because I liked the project and I wanted it to be better, that's how operation systems were built btw, however, it looks like you don't like it when people try to do that, therefore I will leave you to it and wish you the best of luck. |
something is wrong with esmock
please |
Follow up PR after #269