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: use @std prefix for internal module specifiers #24543

Merged
merged 24 commits into from
Jul 25, 2024

Conversation

iuioiua
Copy link
Contributor

@iuioiua iuioiua commented Jul 12, 2024

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.

@iuioiua iuioiua added the ci-draft Run the CI on draft PRs. label Jul 12, 2024
@iuioiua iuioiua changed the title chore: use @std/ prefix for internal module specifiers chore: use @std prefix for internal module specifiers Jul 12, 2024
@iuioiua iuioiua marked this pull request as ready for review July 16, 2024 06:02
Copy link
Member

@dsherret dsherret left a 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
Copy link
Member

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

Copy link
Contributor Author

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?

Copy link
Member

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

Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

@iuioiua iuioiua removed the ci-draft Run the CI on draft PRs. label Jul 18, 2024
Copy link
Member

@bartlomieju bartlomieju left a 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 🙏

@iuioiua iuioiua merged commit f248050 into denoland:main Jul 25, 2024
17 checks passed
@iuioiua iuioiua deleted the std-imports branch July 25, 2024 00:27
dsherret pushed a commit that referenced this pull request Jul 26, 2024
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.
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.

4 participants