Skip to content

Add .isWeakRef() - #165

Merged
sindresorhus merged 21 commits into
sindresorhus:mainfrom
iambinlin:feature/add-weakref
Jun 13, 2022
Merged

sindresorhus merged 21 commits into
sindresorhus:mainfrom
iambinlin:feature/add-weakref

Conversation

@iambinlin

@iambinlin iambinlin commented Mar 2, 2022

Copy link
Copy Markdown
Contributor

@sindresorhus done other PR to replace #164

@iambinlin iambinlin changed the title Add .isWeakRef Add .isWeakRef() Mar 2, 2022
@sindresorhus

Copy link
Copy Markdown
Owner

Tests are failing

Comment thread tsconfig.json Outdated
Comment thread package.json Outdated
@iambinlin
iambinlin requested a review from sindresorhus March 2, 2022 14:36
@iambinlin

Copy link
Copy Markdown
Contributor Author

Tests are failing

Sorry, I haven't checked the problem yet. At present, it seems that only node version 14 is passed.

@iambinlin

Copy link
Copy Markdown
Contributor Author

Tests are failing

image

I found the reason, because JSDOM WeakRef currently supports V14.6.0+ , can I Mock one manually? As a downgrade solution to handle Node versions?

@iambinlin iambinlin mentioned this pull request Mar 3, 2022
@iambinlin

Copy link
Copy Markdown
Contributor Author

Unfortunately, I have not found a solution on how to simulate the behavior of WeakRef. At present, because JSOM simulation is limited to Node V14.6.0+, the test of the lower version fails, so I can only temporarily close this PR, thank you for your understanding

@iambinlin iambinlin closed this Mar 3, 2022
@iambinlin

Copy link
Copy Markdown
Contributor Author

https://github.com/ungap/weakrefs/blob/master/cjs/index.js perhaps it can try mock in node env

@iambinlin iambinlin reopened this Mar 9, 2022
@iambinlin

Copy link
Copy Markdown
Contributor Author

Although the use case must be run on a low version node environment, the minimum requirement for WeakRef is Node V14.6.0, so I don't think it is necessary to create a mock use case for it on a low version Node.

@iambinlin

Copy link
Copy Markdown
Contributor Author

Record: JSDOM@latest will reported error in ci with Node V10.
image

@iambinlin

Copy link
Copy Markdown
Contributor Author

Tests all pass, @sindresorhus 🥳

@sindresorhus

Copy link
Copy Markdown
Owner

Can you fix the merge conflict?

@iambinlin

iambinlin commented Jun 12, 2022

Copy link
Copy Markdown
Contributor Author

Can you fix the merge conflict?

Ok. Had fixed!But I encountered a very strange problem. Line 343 is a blank line, but lint reported an error. I repeatedly checked the code and found no problems. Can I ask for your help.

image

Comment thread source/index.ts Outdated
@sindresorhus

Copy link
Copy Markdown
Owner

I suggest we just embed the WeakRef type so we don't have to require a higher target.

interface WeakRef<T extends object> {
    readonly [Symbol.toStringTag]: "WeakRef";

    /**
     * Returns the WeakRef instance's target object, or undefined if the target object has been
     * reclaimed.
     */
    deref(): T | undefined;
}

@iambinlin

Copy link
Copy Markdown
Contributor Author

I suggest we just embed the WeakRef type so we don't have to require a higher target.

interface WeakRef<T extends object> {
    readonly [Symbol.toStringTag]: "WeakRef";

    /**
     * Returns the WeakRef instance's target object, or undefined if the target object has been
     * reclaimed.
     */
    deref(): T | undefined;
}

done

@iambinlin

Copy link
Copy Markdown
Contributor Author

Can you fix the merge conflict?

Ok. Had fixed!But I encountered a very strange problem. Line 343 is a blank line, but lint reported an error. I repeatedly checked the code and found no problems. Can I ask for your help.

image

Thank you so much, I just noticed that I read the wrong file, it is a lint problem in another file, sorry, sorry for the trouble, I will be more careful next time

@sindresorhus
sindresorhus merged commit e503a9e into sindresorhus:main Jun 13, 2022
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