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 Deno support with Denoify #108

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

garronej
Copy link

@garronej garronej commented Sep 1, 2022

Following up on #33,

As mentioned, it would be cleaner not to track the deno_dist directory on the main branch and publish with an action like tsafe or EVT does, I can help you with that if you want but I'd understand if you'd prefer not messing with your CI.

After merging what's left for you to do is to publish the module on the third party Deno module repository:
Navigate to deno.land/x, click "Add a module" then follow the instruction. Use deno_dist/ as subdirectory when asked.

Best,

@zebp zebp mentioned this pull request Sep 1, 2022
@gvergnaud
Copy link
Owner

Thanks for the PR, I'll take a look at this soon (sorry for the delay, I'm kind of busy lately)

@garronej
Copy link
Author

garronej commented Sep 9, 2022

I know what it is. In your own time!

@gvergnaud
Copy link
Owner

Hey!

I agree it would be preferable not to track the deno_dist build. Since I'm not so familiar with github actions, so I'd be super interested if you want to setup a basic action to publish new version to deno automatically. If it's too much work, I'll just merge this PR.

Quick question, are we still going to need a special build once deno starts supporting npm modules natively as announced here?

garronej added a commit to garronej/ts-pattern that referenced this pull request Sep 15, 2022
@garronej garronej force-pushed the deno_support branch 3 times, most recently from 9cb83ca to c4858c2 Compare September 15, 2022 15:57
@garronej
Copy link
Author

Hi @gvergnaud !

Quick question, are we still going to need a special build once deno starts supporting npm modules natively as announced here?

Good question, theorically no, but I want to use ts-pattern as a dependency for EVT and from a 'marketing' standpoint, offering a Deno module that depends on NPM packages... it's not great.
Also I would like EVT to remain retrocompatible with current and older Deno version.

Regarding the CI I'm very glad you asked!
It's much better not to track the deno_dist and since you do not have a CI workflow already I was able setup it up quickly.

What the CI does:

  • On every commit:
    • It runs the test on Node
    • It checks that, type wise, the Deno distribution is OK
  • If all tests passes it checks if the version number in the package.json have been updated if yes, it release on NPM and update the latest branch that will be used as target for GitHub release tags.

What you need to do:

  • Create a GitHub action Secret NPM_TOKEN that contains your npm token. You can get this token on npmjs.com:

image

image

- Whenever you want to publish a new version you'll just have to update your `package.json` number push and wait.

image

  • Publish the module on the third party Deno module repository:
    Navigate to deno.land/x, click "Add a module" then follow the instruction. Use deno_dist/ as subdirectory when asked.

If you have any question, I'm here to help!

src/patterns.ts Outdated
@@ -16,7 +16,7 @@ import {
GuardExcludeP,
} from './types/Pattern';

export { Pattern };
export type { Pattern };
Copy link
Author

Choose a reason for hiding this comment

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

Deno requires that types be exported as type.

Comment on lines +9 to +11
<a href="https://github.com/gvergnaud/ts-pattern/actions">
<img src="https://github.com/gvergnaud/ts-pattern/workflows/ci/badge.svg?branch=main" alt="CI Workflow" height="18">
</a>
Copy link
Owner

Choose a reason for hiding this comment

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

👍

README.md Outdated Show resolved Hide resolved
@garronej garronej force-pushed the deno_support branch 2 times, most recently from 12cc99f to bee9c30 Compare September 19, 2022 13:54
@garronej
Copy link
Author

garronej commented Oct 9, 2022

Hi @gvergnaud,

A quick up just in case you forgot this PR :)
Let me know if you have any question or concern.

Best,

@gvergnaud
Copy link
Owner

gvergnaud commented Oct 11, 2022

Hey @garronej, I haven't forgotten your PR!

I'm sorry for the delay, I just didn't have the time to read and understand your github action config and I didn't want to merge something I won't be able to maintain.

I have a few questions actually:

Whenever you want to publish a new version you'll just have to update your package.json number push and wait.

  1. I often start by releasing a RC version with the tag @next, how is it going to handle those versions?

  2. Is this action only going to be triggered on the main branch or on all branches?

  3. when I enter ts-pattern in https://deno.land/add_module it says that the name has been taken. Do you know if there is a way to claim this name? It looks like deno doesn't support dashes in names 🤔

image

@garronej
Copy link
Author

garronej commented Oct 11, 2022

Hello @gvergnaud,

I'm sorry for the delay, I just didn't have the time to read and understand your github action config and I didn't want to merge something I won't be able to maintain.

Understandable. Anyway I'll always be there to assist you if you have any problem with the CI setup.

I often start by releasing a RC version with the tag @next, how is it going to handle those versions?

The way this is hanled is, if you update your package.json's version with something like 4.1.0-beta.0 it will create a pre-release on GitHub and release on NPM.

It's basically the same as what you are curently doing but with beta instead of next.

If you want I can update ts-ci so that it work with next as well.

Is this action only going to be triggered on the main branch or on all branches?

It tirggers only on the main branch and on the branches of the repo that have an open PR on main.
Branches of forks that have an open PR on main will only be able to run the tests but not to release (obviously).

It you want this action to run on a branch that has no open PR on main you can simply edit this line, replacing main by the name of the branch.

It looks like deno doesn't support dashes in names 🤔

Yes, Ryan Dahl wanted it this way. You'll have to use ts_pattern or tspattern either way Denoify will be able to automatically resolve your deno distribution.
Examples with a run-exclusive import in EVT automatically replaced by an import of run_exclusive from deno.land/x.

Best regards,

@garronej garronej marked this pull request as draft October 11, 2022 13:13
@garronej
Copy link
Author

I have put this PR in draft while I update ts-ci to better acomodate your current workflow.

@garronej
Copy link
Author

garronej commented Oct 12, 2022

Hi @gvergnaud,

often start by releasing a RC version with the tag https://github.com/next, how is it going to handle those versions?

You can do that by updating the package.json's version to X.Y.Z-rc.U, it will create a pre-release on GitHub and publish on NPM a version X.Y.Z-rc.U with the next tag.

@garronej garronej marked this pull request as ready for review October 12, 2022 20:41
@lilnasy
Copy link

lilnasy commented Dec 18, 2022

I've been using Deno for a while, and the one thing I've needed to do use most modern libraries is adding .ts extension to all the imports, and this project wasn't an exception. typescript#51669 introduces moduleResolution: bundler compiler configuration to permit the same for node projects.

It type checks fine

Screenshot (17)

Can be imported directly from source code

Screenshot (15)

LSP has no issues

Screenshot (14)

I'm wondering if it's worth waiting for typescript to cut the next release instead of fiddling with the tooling here.

@garronej
Copy link
Author

garronej commented Jan 4, 2023

@gvergnaud I agree, considering that ts-pattern has no dependency and isn't using any Node builtins the approach suggested by @lilnasy works.

@garronej garronej closed this Jan 4, 2023
@garronej garronej reopened this Jan 4, 2023
@gvergnaud
Copy link
Owner

Thanks @garronej so sorry for the many back and forths on this PR, I was still hesitant on merging it because of my lack of familiarity with github actions, and the fact that I don't have a lot of time to dedicate the tooling around this repo :/

@garronej
Copy link
Author

No problem @gvergnaud, perfectly understandable.
I just hope ts-pattern will eventually be released on Deno.land one way or the other.
I'm looking forward beeing able to make it a dependency of EVT.

@garronej garronej closed this Jan 12, 2023
@garronej garronej mentioned this pull request Feb 19, 2023
@garronej garronej reopened this Jul 2, 2023
@garronej
Copy link
Author

garronej commented Jul 2, 2023

Hello @gvergnaud,

I've decided to reopen this PR, believing that incorporating Denoify, upon further reflection and after evaluating the alternative suggested by @lilnasy, is the most effective strategy.

@lilnasy seems to agree.

In this updated PR, I've ensured the successful testing of the v5 release in the demo repository.

I comprehend your reservations regarding the introduction of a CI workflow that wasn't authored by you, particularly given your limited familiarity with GitHub Action. However, I've taken the liberty of reopening the discussion for several compelling reasons which might alter your perspective:

  • The addition of an official Deno distribution will delight many users, myself included.
  • Introducing Denoify will not adversely impact the project. It merely adds one more stage to the build process.
  • This workflow is tried-and-true, having been employed across various modules.
  • Should you encounter any difficulties with the workflow, either immediately or in the future, I pledge to be available for prompt resolution within a few hours.

Ultimately, your project operates under your rules. I've made my argument, and it is your decision whether to move forward or not. If you opt to proceed, these instructions remain applicable.

I genuinely hope that my initiative to reopen this PR does not seem excessively persistent.

Best regards,

@roziscoding
Copy link

As a +1, I'm currently writing a plugin for grammY that will benefit greatly from using ts-pattern, and it'd be awesome to be able to use this natively on Deno.

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.

6 participants