-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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: use @std
prefix for internal module specifiers
#24543
Conversation
@std/
prefix for internal module specifiers@std
prefix for internal module specifiers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests sharing a config file is fine, but we should probably update the tools directory to have its own deno.json.
@@ -494,7 +494,7 @@ jobs: | |||
matrix.job == 'test' && | |||
matrix.profile == 'release' && | |||
!startsWith(github.ref, 'refs/tags/')) | |||
run: target/release/deno run -A --unstable ext/websocket/autobahn/fuzzingclient.js | |||
run: target/release/deno run -A --unstable --config tests/config/deno.json ext/websocket/autobahn/fuzzingclient.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should add a deno.json in the tools directory that gets auto-discovered? Then we don't need to add --config
here. It's also strange for these to be using something in tests/config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice suggestion. I didn't know deno.json
auto-discovery worked like that. TIL. Can we not also add a deno.json
to tests
or would that interfere with the tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would most likely interfere we some of the tests - but +1 for adding another deno.json
in tools/
directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In many cases we've traded a relative specifier in the code for a relative specifier when launching the command. It would be nice to have all the tests importing from the fake JSR registry instead of a shared config file like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the best way to vendor the code into tests/registry/jsr/@std
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine for this pass - we're still using a really old version of std, after this lands I think we should figure out a way to easily vendor 1.0 version of std from JSR and then update this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, let's get deno_std
submodule updated ASAP 🙏
This change aims to replace all relative import specifiers targeted at `tests/util/std` with mapped ones (using a `deno.json` file). Towards updating the `std` git submodule.
This change aims to replace all relative import specifiers targeted at
tests/util/std
with mapped ones (using adeno.json
file). Towards updating thestd
git submodule.