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

Use deno info to search for imports #100

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

sigmaSd
Copy link
Contributor

@sigmaSd sigmaSd commented Dec 10, 2022

fix #8

I left couple of notes

This implements the search, and the search gives precise location of the urls, maybe we can in the future use that for replacement.

The tests passes, I did not test more

@sigmaSd
Copy link
Contributor Author

sigmaSd commented Dec 10, 2022

I noticed this requires --allow-run=deno (also known as -A)

@sigmaSd
Copy link
Contributor Author

sigmaSd commented Dec 10, 2022

I also noticed the tests are way slower

@iuioiua
Copy link

iuioiua commented Feb 1, 2023

deno info uses deno_graph under the hood. Perhaps, it'd be better to use that instead of spawning a new process. If you'd like a better idea of how it can be implemented, Deno's Standard Library uses it to check for circular dependencies within the Node compatibility layer here.

@sigmaSd
Copy link
Contributor Author

sigmaSd commented Feb 1, 2023

Thanks @iuioiua I updated the code with deno graph

The tests are not slow anymore, and no new permissions is needed with deno graph

I'm not 100 % confident of the code, but it seems to work and pass tests

This PR implements the search for the import which gives precise location of the urls but this info is not being used currently, maybe in a future pr we can leverage that for url replacement instead of the current regex approach

@pixeleet pixeleet mentioned this pull request Apr 29, 2023
@sigmaSd
Copy link
Contributor Author

sigmaSd commented May 27, 2023

I just noticed that this pr make udd not work on import map anymore

@sigmaSd
Copy link
Contributor Author

sigmaSd commented May 27, 2023

The problem is with bare specifier like $fresh, those don't get resolved

But it seems like deno info can resolve bare specifiers, so I probably need to look how they do it

@sigmaSd
Copy link
Contributor Author

sigmaSd commented May 27, 2023

deno_graph exposes createGraph(..,{resolve?) to override resolution for cases like handling urls from import map, but there is not provided resolver, it needs to be written from scratch it seems, like the deno cli one https://github.com/denoland/deno/blob/d0c5ff42f4b5fa9b848e6ed5af2e480d12f15bda/cli/resolver.rs#L216

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.

use deno info once it can output json
2 participants