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

fix (crypto) Remove fixed version imports #1135

Merged
merged 2 commits into from
Aug 16, 2021

Conversation

LionC
Copy link
Contributor

@LionC LionC commented Aug 15, 2021

This removes two absolute version imports within the crypto bench, as std should always depend on sibling modules (and be sibling-compatible).

There is another fixed dependency within node/_tools on x/compress - this also seems a bit odd but harder to fix without some discussion.

We had a short discussion on Discord about adding a CI step that checks if std does not contain any external dependencies. There are ways to do this with the current (unstable) deno info JSON output, but that might be another discussion altogether, as running / testing a deno module while allowing only local imports might be a more generally useful case.

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

@LionC LGTM. Thanks!

@kt3k kt3k merged commit edcc477 into denoland:main Aug 16, 2021
@kt3k
Copy link
Member

kt3k commented Aug 16, 2021

node/_tools are the internal tools for developing std/node test cases, and not a part of std public API. So I think they should be allowed to depend on external modules when necessary.

For reference, there are also tools in denoland/deno which depends on external modules. https://github.com/denoland/deno/blob/main/tools/wpt/runner.ts

@LionC LionC deleted the fix-absolute-imports branch August 23, 2021 10:54
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