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 support for Deno #209

Closed
wants to merge 1 commit into from
Closed

Conversation

wilmveel
Copy link
Contributor

@wilmveel wilmveel commented Nov 2, 2020

For a side project zod-router we are using zod in combination with deno. To be able to use zod with deno it is necessary to use the full path of the files including the extension for all imports. Would it be possible to publish zod in a way that it is node and deno compatible.

Copy link
Owner

@colinhacks colinhacks left a comment

Choose a reason for hiding this comment

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

Were you able to run the tests in a Deno environment? I'd like some way to verify that this continues to work for Deno, especially if a single extension-less import will break everything.

@@ -9,7 +9,7 @@ type CatcherItem = { type: 'catcher'; catcher: Catcher };
type Items = (FuncItem | CatcherItem)[];

export class PseudoPromise<ReturnType = undefined> {
readonly _return: ReturnType;
readonly _return: ReturnType | undefined;
Copy link
Owner

Choose a reason for hiding this comment

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

Why was this changed?

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 get the following error when undefined is not added

error: TS2564 [ERROR]: Property '_return' has no initializer and is not definitely assigned in the constructor.
  readonly _return: ReturnType;

@@ -1,4 +1,4 @@
import * as z from '..';
Copy link
Owner

Choose a reason for hiding this comment

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

This doesn't have to be ../index.ts?

@wilmveel
Copy link
Contributor Author

wilmveel commented Nov 2, 2020

It seems to be hard to use deno in combination with jest. I have converted all the tests to deno test. They all run green in github actions

@wilmveel
Copy link
Contributor Author

wilmveel commented Nov 2, 2020

@colinhacks colinhacks changed the title .ts Add support for Deno Nov 3, 2020
@colinhacks
Copy link
Owner

Not sure what to do here. I can't merge this into master since now all the tests are written with Deno.test. Ideally there would be a way to run the same tests in both the Node and Deno environments. Also adding the file extension to the imports causes an error in the project's existing TypeScript setup. Not sure how Deno sidesteps this: microsoft/TypeScript#27481

@wilmveel
Copy link
Contributor Author

wilmveel commented Nov 3, 2020

I think we should do this in smaller steps.

  • refactor the test suite into pure functions which can be used in deno and node env
  • I agree that the project should stay an pure typescript project. Wait in what direction the ts community will move regarding deno and the .ts extensions.
  • with roll-up it is possible to add a build step which adds the extensions to the project.
  • add automation that does the build step and runs the test in both environments

@wilmveel
Copy link
Contributor Author

wilmveel commented Nov 3, 2020

denoland/deno#3196

@wilmveel wilmveel force-pushed the v2_deno branch 2 times, most recently from 8863e9f to e10041b Compare November 5, 2020 08:09
@wilmveel
Copy link
Contributor Author

wilmveel commented Nov 5, 2020

I have changed the approach a bit.

Open question is how to publish the deno version. Common in deno land is to push your scr files to github. Only it would be a bit wierd to have the sources duplicated in the project.

@wilmveel wilmveel force-pushed the v2_deno branch 3 times, most recently from d915a1b to 4d23a94 Compare November 6, 2020 16:06
@jeremyBanks
Copy link
Contributor

@wilmveel Thanks for this branch, I'm using it locally to get started with Zod. However, FYI it appears to have broken with this change in Deno 1.5:

This release enables the isolatedModules TypeScript compiler option for all users by default. In the 1.4 release this flag was enabled for users using --unstable.

@wilmveel
Copy link
Contributor Author

wilmveel commented Nov 28, 2020 via email

@colinhacks
Copy link
Owner

@wilmveel what would that entail?

@jeremyBanks
Copy link
Contributor

If I understand correctly (I may not, take this with a grain of salt), the recommended way to deal with this is to use TypeScript 3.8's import type keyword instead of bare import when performing type-only imports, so that it's clear which modules will actually be included at runtime versus only at type-checking-time. However, that requires TypeScript 3.8, and Zod appears to be aiming to support 3.2+.

It looks like there's also an option to change this behaviour in tsconfig, which might be acceptable for my personal projects, but Deno's default tsconfig is used almost everywhere and libraries are generally expected to work under that config.

@jeremyBanks
Copy link
Contributor

jeremyBanks commented Nov 29, 2020

Tried out a few changes 8eb5a47...f381bbe, confirming that this branch works on the current version of Deno if we override the tsconfig, or if we change a few imports over to import type.

It looks like a that drop-in-replacement for Jest has also become broken, but I've submitted a fix: allain/expect#13

@wilmveel
Copy link
Contributor Author

@wilmveel what would that entail?

@wilmveel what would that entail?

https://deno.land/posts/v1.5#stricter-type-checks-in-stable

@wilmveel
Copy link
Contributor Author

I found another way to publish the Deno package via unpkg. Instead of pushing the sources twice to GitHub. I have implemented this example in flock-community/zod-endpoints.

unpkg.com/zod-endpoints/lib/deno/mod.ts

I can change this pr if you like this approach more. Downside you cannot be registered as third party package at deno.land

@jeremyBanks
Copy link
Contributor

jeremyBanks commented Dec 1, 2020

It would be a shame if we couldn't publish Zod to deno.land/x. Zod's zero NPM dependencies make it's a very good candidate for use in Deno (beyond its existing merits), and it would be nice to be able to use it in packages that stay within the deno.land package ecosystem for stability.

I will see if we can work around the import type issue without breaking 3.2 compatibility.

I don't think it's possible to avoid committing the generated Deno duplicate of the code (although if it's seen as too much clutter, they only really need to be there for the tagged commits, and can be deleted in between). We could add an Action to automatically generate and commit the Deno version of the code whenever changes are pushed (or maybe only when sent for pull requests?) to reduce the overhead.

@jeremyBanks
Copy link
Contributor

jeremyBanks commented Dec 1, 2020

@colinhacks You may want to consider registering the zod name on deno.land/x now rather than later, even if Deno isn't supported yet, to avoid having someone else take the name. You don't need to do anything on deno.land, you just need to add a webhook on GitHub:

  • Navigate to the repository you want to add.
  • Go to the Settings tab.
  • Click on the Webhooks tab.
  • Click on the Add webhook button.
  • Enter the URL https://api.deno.land/webhook/gh/zod in the payload URL field.
  • Select application/json as the content type.
  • Select Let me select individual events.
  • Select only the Branch or tag creation event.
  • Press Add webhook.

(We may need to change that URL to https://api.deno.land/webhook/gh/zod?subdir=deno_lib%2F in the future, but I don't think we can use a currently-nonexistent path, so the above works for now.)

@colinhacks
Copy link
Owner

Done! Thanks for the guidance @jeremyBanks.

Btw, Zod v2 is coming out of beta shortly and I've bumped the minimum TypeScript version to 3.7. That may help with these efforts.

This was referenced Dec 3, 2020
@jeremyBanks
Copy link
Contributor

jeremyBanks commented Dec 3, 2020

I've been trying some workarounds for making isolated modules work in 3.7 (without import type), but it's not looking too promising. You can explicitly re-export types, but it's verbose, requires a lot of duplication for generic types, and seems to create visibility issues:

Exported type alias 'TypeOf' has or is using private name ''.ts(4081)

Other workarounds lose necessary type information. I'll look at it a bit more, but I'm not optimistic.


It may not be what Zod needs at this time, but if any other Deno users want something they can use Right Now, I have a branch at #247 that works in Deno and TypeScript 3.8.

@wilmveel
Copy link
Contributor Author

wilmveel commented Dec 4, 2020

@jeremyBanks I see you have created an alternative for denoify. Is it possible to release this script separately? I would like to use it for another project I'm working on.

@jeremyBanks
Copy link
Contributor

@wilmveel I don't really mean to make that deno-library-building script suitable for general use. More general-purpose tools like Denoify already exist, and I don't think I'd be contributing anything new. But I thought it would be easier for Zod development to have a small single-purpose script committed here instead. Feel free to copy my script (I hereby release it to the public domain/cc0), but I'm not sure it would be worth me releasing it as its own thing.

@jeremyBanks
Copy link
Contributor

jeremyBanks commented Dec 6, 2020

It looks like deno bundle (and the upcoming deno compile, which depends on it) currently have a bug, where it converts certain exports into local variables, but some of Zod's exports are context-dependent keywords that aren't allowed as local variables, causing a syntax error.

This is clearly a bug on Deno's side. I'll see if it's already being tracked, or if we can get a fix in the next Deno release.

EDIT: It looks like it's a bug in SWC, which Deno depends on. I've filed an issue. I'm not sure it's within the scope of my abilities to fix.

@colinhacks
Copy link
Owner

@wilmveel @jeremyBanks these efforts looks excellent, tag me if/when this is ready for primetime and I'll deploy to deno.land/x. Are you guys waiting on anything from me, or are we just waiting for upstream bugs to resolve?

@jeremyBanks
Copy link
Contributor

@colinhacks I would suggest we may want to wait for upstream to fix the bundle issue, since that's a pretty significant limitation. (I don't need it, and am happily using this branch for my own project, but it might give some users a disappointing first impression.) Hopefully that will be fixed in the next Deno release, but we'll have to see.

We do not currently have a solution implemented for the import types issue breaking 3.7 compatibility. We could probably do something hacky like adding magic comments that our Deno-converting-script uses to convert appropriate imports from import to import type. I'd be happy to write that, but will probably wait until the bundle issue is fixed, instead of writing a PR that will probably be stale by then.

@colinhacks
Copy link
Owner

colinhacks commented Dec 12, 2020

As it turns out I'll potentially be shipping the beta for Zod 3 soon to fix some architectural issues that have been plaguing transformers: #264. I'm more than happy to bump the minimum version for Zod 3 to TS 4.0, maybe even 4.1, since it'll be in beta for a long time.

But regardless of that I'm happy having different minimum TS version requirements for the Deno and non-Deno versions. (This only applies if we're able to figure out how to generate the import type statements with a codemod.) We can just add a notice to the README for Deno users. If TS 3.7 is giving you trouble, don't worry about it.

@colinhacks
Copy link
Owner

It looks like deno bundle (and the upcoming deno compile, which depends on it) currently have a bug, where it converts certain exports into local variables, but some of Zod's exports are context-dependent keywords that aren't allowed as local variables, causing a syntax error.

Since Zod 3 can have certain breaking changes, could we just get rid of the offending syntax? If its as simple as renaming z.instanceof to z.instanceOf I'm more than happy to do that. Are there other cases where this bundle bug was causing issues?

Also is there any other reasons your PR is still a draft? If not I'll merge it into my v3 branch.

@jeremyBanks
Copy link
Contributor

jeremyBanks commented Dec 13, 2020

@colinhacks Thanks for the feedback.

As it turns out I'll potentially be shipping the beta for Zod 3 soon to fix some architectural issues that have been plaguing transformers: #264. I'm more than happy to bump the minimum version for Zod 3 to TS 4.0, maybe even 4.1, since it'll be in beta for a long time.

Cool. In that case, I would suggest bumping it the minimum TypeScript version to 3.8 for Zod 3. If we used a codemod or regex to make the import changes only for the Deno build, that could work for Deno, but would lose support for isolatedModules in non-Deno TypeScript environments. I think it would be nice to have that support, since it unlocks easier parallel compilation and compatibility for tools like SWC, which I hope will have more use than TypeScript 3.7 in the future 🤞 . I've submitted an updated PR targeting v3 with this change: #269

Since Zod 3 can have certain breaking changes, could we just get rid of the offending syntax? If its as simple as renaming z.instanceof to z.instanceOf I'm more than happy to do that. Are there other cases where this bundle bug was causing issues?

Also is there any other reasons your PR is still a draft? If not I'll merge it into my v3 branch.

I think that might be all we need to change. I'll give it a try, and if it works, send an updated PR for Deno support targeting the v3 branch. (edit: it didn't work but I still updated #269 to add Deno compatibility.)

@colinhacks
Copy link
Owner

Closing in favor of #269

@colinhacks colinhacks closed this Jan 12, 2021
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.

3 participants