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.

Sign up for GitHub

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

Jump to bottom

Only use setImmediate if it is defined #137

Merged
merged 6 commits into from
Feb 13, 2024
Merged

Only use setImmediate if it is defined #137

merged 6 commits into from
Feb 13, 2024

Conversation

jmaroeder
Copy link
Contributor

This PR changes the strategy used to determine if setImmediate is available, which should allow the library to work correctly on other non-Node runtimes such as cloudflare workers.

Fixes #136

mcollina
mcollina reviewed Feb 10, 2024
View reviewed changes
Copy link
Owner

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

If the target is cloudflare workers, could you add some sort of test to verify it works correctly there?

@jmaroeder
Copy link
Contributor Author

could you add some sort of test to verify it works correctly there?

Possibly, though it may involve a lot of extra setup/dependencies. I can look into it more soon, probably early next week

@jmaroeder
Copy link
Contributor Author

@mcollina I've added a test that uses wrangler, which is the documented way to use the workerd runtime in tests. I also verified that it failed this test when running with the original implementation in storage/memory.js, and it passes now.

mcollina
mcollina approved these changes Feb 13, 2024
View reviewed changes
Copy link
Owner

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

mcollina
mcollina reviewed Feb 13, 2024
View reviewed changes
test/storage-memory.cloudflare.test.js Outdated

const sleep = promisify(setTimeout)

describe('storage memory (cloudflare)', async () => {
Copy link
Owner

@mcollina mcollina Feb 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

can you please skip this in Node.js v18.6? it's failing on CI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Skipped in c774406

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it's still not skipping the suite in 18 - perhaps before is still being run for the suite? I'm sorry, this is my first experience with node:test, and I'm not entirely certain how to solve this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although it appears to be documented for v18, I couldn't get the option to work when running 18.19.0 locally. I just put the whole describe in an if block in d1a4ed7, and ran it locally against v18.19.0

simone-sanfratello
simone-sanfratello requested changes Feb 13, 2024
View reviewed changes
test/storage-memory.cloudflare.test.js Outdated
await worker.stop()
})

test('able to invalidate without error', async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test doesn't assert that invalidation works, since the function in the helper returns always an empty object, doesn't it?

What should or should not happen? Throwing/not throwing an error, returning a different value, other?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should get a response from the cloudflare worker without encountering an error. The delay is simply to trigger invalidation in the worker.mjs code. I have changed the description of the test to clarify this intent in fb9dda1

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, more clear now. Could you use assert.doesNotThrow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used doesNotReject (since it's async) in a3741e5

mcollina
mcollina approved these changes Feb 13, 2024
View reviewed changes
Copy link
Owner

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit f347977 into mcollina:main Feb 13, 2024
54 checks passed
@liuhanqu
Copy link
Contributor

liuhanqu commented Dec 18, 2024 •
edited
Loading

execute command npm install would have error like below

npm ERR! code ETARGET
npm ERR! notarget No matching version found for [email protected].
npm ERR! notarget In most cases you or one of your dependencies are requesting
npm ERR! notarget a package version that doesn't exist.

It's because wrangler have peerDepencedies, but the unenv-nightly is not matched right version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@simone-sanfratello simone-sanfratello simone-sanfratello approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Some non-browser runtimes do not provide setImmediate
4 participants
@jmaroeder @liuhanqu @mcollina @simone-sanfratello