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

Add browser support #48

Merged
merged 34 commits into from Feb 10, 2023
Merged

Conversation

mateonunez
Copy link
Sponsor Contributor

@mateonunez mateonunez commented Jan 28, 2023

This PR would introduce the browser support for async-cache-dedupe as discussed in #47.

Context

This library is designed to run on Node.js. With support for Chrome, Firefox, Safari, and Edge., the async-cache-dedupe module can be extended between platforms and, in the near future, to other runtimes.

  • Test Runner.
  • Browser runner.
  • Prepare runner.
  • ESBuild.
  • Rollup.
  • Webpack.
  • Browserify.
  • Vite.
  • Chrome.
  • Firefox.
  • Safari.
  • Edge.
  • Tests. (util was not tested)
  • Documentation.
  • CI.

Test

I've added a test system to run all tests in the following browsers: Chrome, Safari, Edge, and Firefox. I also add support for the following bundlers: Esbuild, Webpack, Browserify, and Rollup.

To test correctly each bundler you need to run the following commands:

npm run test:prepare [esbuild|rollup|browserify|webpack]

This command will generate a JS file bundled and move it into a temporary folder.

To run the tests you should run the following command:

npm run test:browser [chrome|firefox|safari|edge]

This gonna run an index.html file that imports the generated suites containing tests. All the output will be streamed from the console of the browser to your Node console.

CI

The CI was added to run the browser tests in the following OS: ubuntu, windows, and macos. Each OS tests all the supported browsers with the supported bundlers. I've skipped the Safari tests on Ubuntu and Windows.

Caveats

There are some limitations to running this module on the client side.

  • Redis Storage will not available on the client side. (throws an exception)

@mcollina
Copy link
Owner

Good work! I think we might want to support Vite too.

I think we should not try to bundle https://github.com/mcollina/async-cache-dedupe/blob/main/src/storage/redis.js at all.

@mateonunez
Copy link
Sponsor Contributor Author

mateonunez commented Jan 30, 2023

Hey @mcollina, thanks for the pre-review 😁

I've added Vite support as well and removed redis.js from bundles. The PR is still in draft because tests fail randomly. I need to investigate the error. I think it's because of timers, setImmediate, or now method. I'll let you know as soon as possible.

@mateonunez mateonunez marked this pull request as ready for review February 1, 2023 09:42
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.

Can you just remove the skipped tests?

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.

A lot of tests here are duplication of the Node.js ones. Could they be imported/loaded instead of copying them over?

@mateonunez
Copy link
Sponsor Contributor Author

A lot of tests here are duplication of the Node.js ones. Could they be imported/loaded instead of copying them over?

Oh, I've tried that way... but I realized there are no shims for async_hooks (used by tap).

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
Copy link
Owner

mcollina commented Feb 1, 2023

let's see what @simone-sanfratello says!

package.json Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@simone-sanfratello
Copy link
Collaborator

Nice work! Looks fine so far.
Please remove all the // istanbul ignore and provide meaningful tests

@simone-sanfratello
Copy link
Collaborator

All good with the latest changes! 👏

One last thing please: remove test:prepare script and run it in test:browser if needed

@mateonunez
Copy link
Sponsor Contributor Author

Hey, @simone-sanfratello. Thank you so much for the review and for your support. I've made all changes you requested.

@simone-sanfratello
Copy link
Collaborator

Well done!

@mcollina mcollina merged commit 341be35 into mcollina:main Feb 10, 2023
@mateonunez mateonunez deleted the feat/browser-support branch February 10, 2023 17:17
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.

None yet

3 participants