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

use jsr modules where possible and upgrade fresh version #684

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

Conversation

ycmjason
Copy link

@ycmjason ycmjason commented Sep 8, 2024

Hello there,

Decided to do some migration of packages to JSR modules and upgrading fresh versions.

Do let me know if there's any modifications I should make. I am not sure how the blog plugin should be typed.. as i can't find any references to location and projectLocation in the official API.

p.s. I am quite new to the deno community so I am not sure with a lot of conventions / best practices. Please feel free to guide me through this. 🙏

.vscode/settings.json Outdated Show resolved Hide resolved
deno.json Outdated Show resolved Hide resolved
deno.json Outdated Show resolved Hide resolved

export function blog(): Plugin {
export function blog(): Plugin & { location: string; projectLocation: string } {
Copy link
Author

Choose a reason for hiding this comment

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

might need some help on this. I couldn't seem to find any documentation about the location and projectLocation options. So I am not sure what to do here.

Copy link
Contributor

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! Glad to see this done. Few suggestions.

.vscode/settings.json Outdated Show resolved Hide resolved
e2e_test.ts Outdated
import { isRedirectStatus, STATUS_CODE } from "$std/http/status.ts";
import { resolvesNext, returnsNext, stub } from "$std/testing/mock.ts";
} from "@std/assert";
import { isRedirectStatus, STATUS_CODE } from "@std/http";
Copy link
Contributor

Choose a reason for hiding this comment

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

Using the most-specific module ensures that only the code we use is downloaded, keeping imports trim.

Suggested change
import { isRedirectStatus, STATUS_CODE } from "@std/http";
import { isRedirectStatus, STATUS_CODE } from "@std/http/status";

Copy link
Author

Choose a reason for hiding this comment

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

wouldn't tree shaking handle this?

e2e_test.ts Outdated
@@ -1,4 +1,5 @@
// Copyright 2023-2024 the Deno authors. All rights reserved. MIT license.
/// <reference lib="deno.unstable" />
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be needed.

Suggested change
/// <reference lib="deno.unstable" />

Copy link
Author

Choose a reason for hiding this comment

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

ya removing this line seem to make test to fail. would love more guidance on how to fix this. thanks!

deno.json Outdated
"tailwindcss/plugin": "npm:/tailwindcss@3.4.1/plugin.js",
"$std/": "https://deno.land/std@0.208.0/",
"stripe": "npm:/stripe@13.5.0",
"@std/assert": "jsr:@std/assert@^1.0.4",
Copy link
Contributor

Choose a reason for hiding this comment

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

@std follows SemVer. For packages with version >= 1, we should set versions like this to continue to get fixes but not introduce breaks. Ditto for other imports like this.

Suggested change
"@std/assert": "jsr:@std/assert@^1.0.4",
"@std/assert": "jsr:@std/assert@^1.0",

deno.json Outdated
"$std/": "https://deno.land/std@0.208.0/",
"stripe": "npm:/stripe@13.5.0",
"@std/assert": "jsr:@std/assert@^1.0.4",
"@std/datetime": "jsr:@std/datetime@^0.225.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"@std/datetime": "jsr:@std/datetime@^0.225.2",
"@std/datetime": "jsr:@std/datetime@^0.225",

import { assertEquals } from "$std/assert/assert_equals.ts";
import { STATUS_CODE } from "$std/http/status.ts";
import { returnsNext, stub } from "@std/testing/mock";
import { assertEquals } from "@std/assert";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { assertEquals } from "@std/assert";
import { assertEquals } from "@std/assert/equals";

import { STATUS_CODE } from "$std/http/status.ts";
import { returnsNext, stub } from "@std/testing/mock";
import { assertEquals } from "@std/assert";
import { STATUS_CODE } from "@std/http";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { STATUS_CODE } from "@std/http";
import { STATUS_CODE } from "@std/http/status";

utils/http.ts Outdated
@@ -1,5 +1,5 @@
// Copyright 2023-2024 the Deno authors. All rights reserved. MIT license.
import { RedirectStatus, STATUS_CODE } from "$std/http/status.ts";
import { RedirectStatus, STATUS_CODE } from "@std/http";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { RedirectStatus, STATUS_CODE } from "@std/http";
import { RedirectStatus, STATUS_CODE } from "@std/http/status";

import { assert, assertEquals, assertRejects } from "$std/assert/mod.ts";
import { STATUS_CODE } from "$std/http/status.ts";
import { assert, assertEquals, assertRejects } from "@std/assert";
import { STATUS_CODE } from "@std/http";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { STATUS_CODE } from "@std/http";
import { STATUS_CODE } from "@std/http/status";

utils/stripe.ts Outdated
@@ -1,6 +1,6 @@
// Copyright 2023-2024 the Deno authors. All rights reserved. MIT license.
import Stripe from "stripe";
import { AssertionError } from "$std/assert/assertion_error.ts";
import { AssertionError } from "@std/assert";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { AssertionError } from "@std/assert";
import { AssertionError } from "@std/assert/assertion-error";

"kv_oauth/": "https://deno.land/x/deno_kv_oauth@v0.9.1/",
"preact": "https://esm.sh/preact@10.19.2",
"preact/": "https://esm.sh/preact@10.19.2/",
"stripe": "npm:/stripe@13.5.0",
"tabler_icons_tsx/": "https://deno.land/x/tabler_icons_tsx@0.0.4/tsx/",
Copy link
Author

Choose a reason for hiding this comment

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

also a note on this, we can switch this to:

"@tabler/icons-preact": "npm:@tabler/icons-preact@^3.14.0",

The module we are using is deprecated: https://github.com/hashrock/tabler-icons-tsx?tab=readme-ov-file#deprecated

Copy link
Author

Choose a reason for hiding this comment

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

happy to do this in this PR or a seperate one

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's do that in a separate PR 🙂

@ycmjason ycmjason requested a review from iuioiua September 9, 2024 08:47
Copy link
Contributor

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

Great! Thank you for your contribution! 🙂

"kv_oauth/": "https://deno.land/x/deno_kv_oauth@v0.9.1/",
"preact": "https://esm.sh/preact@10.19.2",
"preact/": "https://esm.sh/preact@10.19.2/",
"stripe": "npm:/stripe@13.5.0",
"tabler_icons_tsx/": "https://deno.land/x/tabler_icons_tsx@0.0.4/tsx/",
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's do that in a separate PR 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify my previous statement, importing from root modules (i.e. @std/assert) for test code is fine. It's usually production code that we want to keep trim. It's no big deal, either way.

@iuioiua
Copy link
Contributor

iuioiua commented Sep 10, 2024

Please allow me to look into these CI failures once I get a chance.

@ycmjason
Copy link
Author

@iuioiua I am curious to understand about specifying modules. I understand that we want to keep things trim, but wouldn't tree shaking handle that for us?

@iuioiua
Copy link
Contributor

iuioiua commented Sep 11, 2024

That's a fair assumption. However, Deno currently doesn't do this. You can test by having the following.

// deps.ts
export { STATUS_CODE } from "jsr:@std/http";

Run deno info deps.ts. You'll see all modules within @std/http are pulled in. Then, change the script to the following.

// deps.ts
export { STATUS_CODE } from "jsr:@std/http/status";

Now, running deno info deps.ts again, notice that only jsr:@std/http/status is pulled in. This fact remains even when we use an import map.

If you think this is worth having, please open an issue in the runtime repo with the suggestion.

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.

2 participants