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

chore: update for deno 2 #662

Merged
merged 2 commits into from
Sep 9, 2024
Merged

chore: update for deno 2 #662

merged 2 commits into from
Sep 9, 2024

Conversation

load1n9
Copy link
Contributor

@load1n9 load1n9 commented Sep 6, 2024

Description

Related Issues

Check List

  • Have you read the
    CODE OF CONDUCT
  • Have you read the document
    CONTRIBUTING
    • One pull request per feature. If you want to do more than one thing,
      send multiple pull request.
    • Write tests.
    • Run deno fmt to fix the code format before commit.
    • Document any change in the CHANGELOG.md.

@@ -28,7 +28,7 @@ jobs:
run: deno lint

- name: Run tests
run: deno task test
run: deno run test
Copy link
Contributor

@marvinhagemeister marvinhagemeister Sep 6, 2024

Choose a reason for hiding this comment

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

FYI: Changing this is more personal preference. The task command will be the same in Deno 2 as it is today. I guess I'm just trying to get a feel if people like deno run more or if our messaging at Deno sent the wrong signal in that we would be deprecating deno task or something. If so, please let us know 😅

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 see, what do you think we should have it as

Copy link
Member

@oscarotero oscarotero Sep 7, 2024

Choose a reason for hiding this comment

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

Honesty, I don't like this change in Deno because it opens the door to inconsitencies and makes the API even more complicated and confused than it's currently:

  • deno run can run files and tasks
  • deno ... only can run files but not tasks
  • what does deno run npm:foo run? the npm:foo package or the npm:foo task?
  • I have been eagerly waiting years for this Support deno run resolving bare specifiers with import map denoland/deno#19955 in order to simplify the lume task, but now, if it's merged it will create more inconsistencies. For example, what does deno fmt run? the fmt bare specifier from import maps (if exists) or the fmt command of Deno?

deno task ... and deno run ... were clear and easy to understand. You don't have to do ALL what Node/NPM do.

@oscarotero
Copy link
Member

thank @load1n9 !!

In addition to the deno task/deno run CLI changes (of those I've expressed by disagreement, but I suppose it's how Deno is going to work until now, so I have to deal with it), I don't see any significant change, only code style. So I don't understand what this PR is trying to fix.

@oscarotero oscarotero merged commit 6be7c48 into lumeland:main Sep 9, 2024
3 checks passed
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