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

Proposal: A tool for generating a deno-optimized version of the driver #830

Merged
merged 1 commit into from
Jan 10, 2022

Conversation

bradenmacdonald
Copy link
Contributor

@bradenmacdonald bradenmacdonald commented Dec 23, 2021

This PR contains a script which I used to auto-generate a Deno version of neo4j-driver-lite. If you are interested in officially supporting Deno, this is one way to do so.

Background

  • Deno is an increasingly popular alternative to Node.js that is built with Rust and natively supports TypeScript
  • Because Deno supports web APIs, the "browser" version of the JavaScript driver does work with Deno (though I haven't tested it extensively). But it's preferable to have an optimized version that includes full TypeScript typings.
  • I had previously mentioned my port of driver version 4.3 on the forum and was told there was some interest from you guys in supporting Deno. So here is one approach. If you'd like to support Deno, I think this is the easiest way, but there are of course other approaches. If you want to close this PR and pursue another path or don't want to support Deno for now, it's totally fine with me.

Prerequisites

While working on Deno, I found and fixed a bunch of other issues in the codebase. To make the review easier, I have separated them into smaller, simpler pull requests:

This PR includes all the commits from those PRs because they're required. Once they are merged, the diff of this PR will be much smaller (really just the one commit 73981b1).

Whether or not you are interested in merging this PR, I do hope you can merge those fix PRs, as it will make it much easier to create a Deno port in the future.

Approach

See this link for the new README that explains the approach:

This folder contains a script which can auto-generate a version of neo4j-driver-lite that is fully compatible with Deno, including complete type information.

The resulting driver does not use any dependencies outside of the Deno standard library.

The script will:

  1. Copy neo4j-driver-lite and the Neo4j packages it uses into a subfolder here called lib.
  2. Rewrite all imports to Deno-compatible versions
  3. Replace the "node channel" with the "browser channel"
  4. Test that the resulting driver can be imported by Deno and passes type checks

Demo

The resulting code that is auto-generated by this script can be seen at https://deno.land/x/neo4j_lite_client@4.4.1-preview . It should work identically to neo4j-driver-lite.

Instructions for testing it out are in the README in this PR and also in the README at https://deno.land/x/neo4j_lite_client@4.4.1-preview

Known issues

I didn't have time to figure out how to test this driver using the test suite. But the code is 99% the same as the neo4j-driver-lite code (which is tested), it's fully type-checked by TypeScript as part of the build process, and I've been using this in a large Neo4j project (via Vertex Framework) without issues.

Other Thoughts

Thanks for having a nicely structured codebase and keeping the dependencies to a minimum! It made this port relatively easy :)

@bigmontz
Copy link
Contributor

Hi @bradenmacdonald,

thanks for you contribution. For your PR be able to be reviewed, you need to read and sign Neo4j's CLA (https://neo4j.com/developer/cla/).

@bradenmacdonald
Copy link
Contributor Author

bradenmacdonald commented Dec 28, 2021

@bigmontz I did, but I did it just before submitting these PRs so maybe your records haven't been updated yet.

@bigmontz
Copy link
Contributor

@bradenmacdonald, found it.

I will take a look at your previous PRs.

@bigmontz
Copy link
Contributor

Hi @bradenmacdonald,

I've merged your others PRs. Could you please rebase this PR?

I will find a moment next week for setting up a Deno environment test the changes you did in this PR.

Many thanks.

@bigmontz bigmontz self-requested a review December 30, 2021 15:10
@bradenmacdonald
Copy link
Contributor Author

@bigmontz Sure thing. Rebased :)

@bigmontz bigmontz changed the base branch from 4.4 to 5.0 January 7, 2022 14:50
Copy link
Contributor

@bigmontz bigmontz left a comment

Choose a reason for hiding this comment

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

Great contribution, @bradenmacdonald.

It's going to be merged to 5.0, since I'd like to setup the integration test suite before release it.

Many thanks, again.

@bradenmacdonald
Copy link
Contributor Author

Makes sense, thanks! Cheers :)

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

2 participants