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

refactor: remove deno.run #1036

Merged
merged 18 commits into from
Mar 9, 2023
Merged

refactor: remove deno.run #1036

merged 18 commits into from
Mar 9, 2023

Conversation

sylc
Copy link
Contributor

@sylc sylc commented Feb 17, 2023

remove deno.run in favor of Deno.Command

@@ -11,7 +11,7 @@ export interface JSXConfig {
let esbuildInitialized: boolean | Promise<void> = false;
async function ensureEsbuildInitialized() {
if (esbuildInitialized === false) {
if (Deno.run === undefined) {
if (Deno.Command === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

may you consider to check it by typeof??

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 think that in Deno, and other recent js runtime, you cannot anymore re-defined undefined and so using typeof is not required. tested in repl

$ deno
Deno 1.30.3+e0074b5
exit using ctrl+d, ctrl+c, or close()
REPL is running with all permissions allowed.
To specify permissions, run `deno repl` with allow flags.
> globalThis.undefined = 1
Uncaught TypeError: Cannot assign to read only property 'undefined' of object '#<Window>'
    at <anonymous>:2:22

Is there any other reasons to use typeof ?

@jsaavedrazuniga
Copy link
Contributor

The actions has failed because Deno.Command is unstable, please add --unstable flag to Ci workflows.
Failed ci workflow

@deer
Copy link
Contributor

deer commented Feb 20, 2023

Deno.Command is going to be stabilized soon. For context: denoland/deno#17628. Seems to be targeted for 1.31 (unless I can't read properly).

@lino-levan
Copy link
Contributor

Deno.Command has been stabilized. Any chance this could get merged @hashrock?

www/main_test.ts Outdated
@@ -2,16 +2,18 @@ import { assertEquals } from "$std/testing/asserts.ts";
import { TextLineStream } from "$std/streams/delimiter.ts";

Deno.test("CORS should not set on GET /fresh-badge.svg", {
sanitizeOps: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

is there any way to avoid using sanitizeOps: false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hashrock i have pushed a change. PTAL thanks.

src/server/bundle.ts Outdated Show resolved Hide resolved
src/server/deps.ts Outdated Show resolved Hide resolved
tests/cli_test.ts Outdated Show resolved Hide resolved
@lucacasonato lucacasonato enabled auto-merge (squash) March 5, 2023 09:57
auto-merge was automatically disabled March 5, 2023 12:05

Head branch was pushed to by a user without write access

@sylc
Copy link
Contributor Author

sylc commented Mar 8, 2023

@lucacasonato @hashrock Could this be merged? (the issue blocking the previous auto-merge has now been resolved). thanks

@hashrock hashrock merged commit 7e72fa2 into denoland:main Mar 9, 2023
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.

7 participants