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

Is protobuf-es compatible with Deno? #293

Open
taras opened this issue Nov 8, 2022 · 11 comments
Open

Is protobuf-es compatible with Deno? #293

taras opened this issue Nov 8, 2022 · 11 comments

Comments

@taras
Copy link

taras commented Nov 8, 2022

I came across protobuf-es announcement and it looks great. The analysis was excellent as well. Since you have native ES support, have you tried it Deno? Thank you, keep up the great work.

@taras taras changed the title Any interested in Deno support? Is protobuf-es compatible with Deno? Nov 8, 2022
@timostamm
Copy link
Member

Hey Taras, we are all very excited about Deno. Generally, our runtime should work fine with Deno because we only use a few standard APIs. But currently, there are a couple of complications:

  1. Our runtime library @bufbuild/protobuf is only published on npmjs.com
  2. The generated code imports from the runtime via from "@bufbuild/protobuf"
  3. The generated code imports other generated code from "./foo.js", where Deno expects from "./foo.ts"

As a concrete example, we generate this:

import {Message} from "@bufbuild/protobuf";
import {Foo} from "./foo.js";

As far as I'm aware, points 1 and 2 can be worked around by the user with an import map, but point 3 is a bit more tricky. We do have some ideas to tackle it in a generic way, but we'd love to get some feedback from actual Deno users.

Let's say we would generate the following:

import {Message} from "https://cdn.skypack.dev/@bufbuild/protobuf";
import {Foo} from "./foo.ts";

Would that be sufficient to run in Deno?

I'd be also very curious how you run a plugin with Deno. Would you need an equivalent to this script specific to Deno, or are there ways to invoke an npm package "bin"?

@taras
Copy link
Author

taras commented Nov 9, 2022

we are all very excited about Deno. Generally, our runtime should work fine with Deno because we only use a few standard APIs.

Sweet!

I'd be also very curious how you run a plugin with Deno. Would you need an equivalent to this script specific to Deno, or are there ways to invoke an npm package "bin"?

@cowboyd how do you think this should work based on what we've been doing with Deno and lessons learned from GraphQL?

@cowboyd
Copy link

cowboyd commented Nov 9, 2022

@timostamm On point (3), it might be a non-issue? That's because importing plain JavaScript sources in Deno "just works." In fact, the entire reason Deno requires the file extension is so that it know whether to invoke the TypeScript compiler or not.

As you said, (1) and (2) can be worked around using import maps, but most npm modules are available on one of the "un-packagers" such as https://esm.sh or https://skypack.dev

Our approach is to write the source code in Deno itself because its such a great developer experience (it is very similar to go-lang) and then build the Npm package as a downstream artifact.

@timostamm
Copy link
Member

@cowboyd, I think I never tried it with generated .js and .d.ts files 🤔

Yup, it works fine, just with an import map. I can make a request to our demo service with a connect-web client.
See this gist.

Will this still properly lint (including typical IDE setups)? Do you think there are any other problematic areas?

@NfNitLoop
Copy link

This seems to be working well for me. I've hacked together a Deno task here that:

  • uses an embedded npm package(.json) to install protoc-gen-es
  • calls protoc w/ options to use protoc-gen-es
  • fixes up the import path in the result to use a module from esm.sh.

The result gives code that I'm able to use directly in Deno. 🎉 (And later I'm going to see if this allows me to also use dnt to also publish this client as an npm package.)

Minor feature requests along the way:

Instead of

  itemType: {
    value: Post;
    case: "post";
  } | {
    value: Profile;
    case: "profile";
  } | {
    value: Comment;
    case: "comment";
  } | { case: undefined; value?: undefined } = { case: undefined };

Wouldn't you still get the same type safety with this?

 itemType: {
    post: Post;
  } | {
    profile: Profile;
  } | {
    comment: Comment;
  } | {} = {};

@NfNitLoop
Copy link

Ah yes, one more oddity to point out:

Here, I discovered that the code tries to access an environment variable named BUF_BIGINT_DISABLE. I expect this is a thing in Node. In Deno, attempting to read an environment variable stops the application to prompt the user for permissions.

@unicomp21
Copy link

Crossing my fingers, would be awesome if this works!

@jcready
Copy link

jcready commented Dec 5, 2022

Wouldn't you still get the same type safety with this?

 itemType: {
    post: Post;
  } | {
    profile: Profile;
  } | {
    comment: Comment;
  } | {} = {};

No.

type exampleOneof = {
  foo: string;
} | {
  bar: number;
} | {
  baz: boolean;
} | {};

declare var example: exampleOneof;

// Can set excess properties
example = { excess: 'property' }; // no error

// Can set INCORRECT values 😱
example = { foo: 1 }; // no error

// Can set MULTIPLE fields 😱
example = { foo: 'both are set', bar: 1 }; // no error

// Can't check which is set using property access
if (example.foo) {}
//           ^ Property 'foo' does not exist on type 'exampleOneof'.
//             Property 'foo' does not exist on type '{}'.(2339)
if (typeof example.foo !== 'undefined') {}
//                  ^ Property 'foo' does not exist on type 'exampleOneof'.
//                    Property 'foo' does not exist on type '{}'.(2339)

// Using `hasOwnProperty()` doesn't refine
if (example.hasOwnProperty('foo')) { example.foo }
//                                            ^ Property 'foo' does not exist on type 'exampleOneof'.
//                                              Property 'foo' does not exist on type '{}'.(2339)

@NfNitLoop
Copy link

For those interested, I got this working "in Deno", here:
https://github.com/NfNitLoop/feoblog-client/tree/aeb18d564909e99f37b3a14d2090abbdc2f1c322/private/protobuf

The "in Deno" is in quotes because I'm currently temporarily using npm to install protobuf-es into a local directory, then use that with protoc to convert my .proto file into .ts. Then it works fine from Deno, and also survived the translation by DNT back into an npm package. This lets my Deno package be my canonical source, but I get to use it from npm too when I have to. :p


@jcready -- thanks for the example. I forgot how lax {} is in TypeScript. I've got a better suggestion over in #337. :)

@smaye81
Copy link
Member

smaye81 commented May 24, 2023

Adding here for posterity also. When Deno support is added, we should support allowing rewrite_imports to rewrite to URLs.

See #483 for the original issue requesting this.

@timostamm
Copy link
Member

Deno v2 supports importing from npm packages with the "npm:" specifier. We're updating the plugin option rewrite_imports to support the prefix in #998 (see #992 for an example).

Deno expects the extension .ts for file imports, which is supported with the plugin option import_extension=ts. Going forward, it may be worth it to support target=deno to automatically set target=ts, import_extension=ts, and rewrite_imports to add the npm specifier and a version for @bufbuild/protobuf.

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

No branches or pull requests

7 participants