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

Create a new Specifier type for JavaScript module specifiers #3818

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hansl
Copy link
Contributor

@hansl hansl commented Apr 17, 2024

This PR does not change any of the current API. I wanted to see if people preferred doing it as part of this PR (thus larger in scope), or doing a review of this new type, then as a follow up update the related APIs.

My thoughts after playing with modules;

  • There is friction between what is a Path, and what is a module specifier. To remove that friction and simplify the API surface, I suggest that all Path from Source, Module and their relative types be moved to using this new Specifier/OwnedSpecifier type.
  • Paths should only be used when working with the file system. That is, a Path is only necessary in a ModuleLoader's implementation, and should never be part of the interface. That way the API is platform independent, and concerns are separated between resolution/namespacing and actually pointing at files. Same goes for URLs (Path being technically a URL). These are implementation specific and should not be part of the API. Having easy conversions from one to the other would make it trivial anyway.
  • Source should have a Specifier, Scripts should take the specifier and make a copy of it, etc.
  • Traces will show up just fine in a single namespace, but we should avoid storing relative specifiers (relative to what). We should see absolute paths for modules, regardless of how they were imported, so import './path' resolving to /some/path.js (see below), should show up in traces as /some/path.js. This would be part of the follow up PR.
  • There should be an invariant that two absolute specifiers should ALWAYS resolve to the same module. AFAICT, that is true of all JavaScript module implementation (can't remember if it's in the spec). Relative specifiers should be resolved to become absolute, then follow the same invariant.
  • I would suggest having a RevoledSpecifier or something which is guaranteed at creation to be absolute and resolved. It can Deref<Specifier> or Deref<OwnedSpecifier>. That way the type system will make sure you cannot create a module with a relative path (relative to what?!?). When creating a module, you need to pass a referrer or a resolved specifier.

Some extra curricular suggestions:

  • There should be some interaction between a resolver and a loader, but these two concerns should likely be separated. I'm not going to do that, but having something like a NodeModuleResolver that doesn't have to also be a loader would be good.
  • Loaders likely should have some awareness of what specifiers are available, and allow queries and (ideally) listing where possible. Query requires that the trait default implementation of ModuleLoader::get_module, and that this method be implemented correctly.
  • I didn't want to get into URLs just yet (or at all). This is not a use case I care about right now. But this new type would also help there, as resolving relative to an existing specifier is made trivial.

Copy link

codecov bot commented Apr 17, 2024

Codecov Report

Attention: Patch coverage is 76.99531% with 49 lines in your changes are missing coverage. Please review.

Project coverage is 50.41%. Comparing base (6ddc2b4) to head (bf0eedc).
Report is 137 commits behind head on main.

Files Patch % Lines
core/engine/src/module/specifier.rs 76.99% 49 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3818      +/-   ##
==========================================
+ Coverage   47.24%   50.41%   +3.16%     
==========================================
  Files         476      460      -16     
  Lines       46892    45179    -1713     
==========================================
+ Hits        22154    22776     +622     
+ Misses      24738    22403    -2335     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jedel1043
Copy link
Member

jedel1043 commented Apr 17, 2024

I'm trying to think about the implications of always having absolute paths for modules and scripts. Right now, I can think of:

  • Users who don't want to use absolute paths on traces for security reasons.
  • Users who want to embed Boa in a system that doesn't have a filesystem (not possible now, WIP),
    but that want to expose some relative paths to the host project for debugging purposes.

Committing to absolute paths would be very bad for both use cases.

Maybe we can just change specifiers to store a str, then the loaders would be the ones to decide if they want to parse unix paths/windows paths/urls?

@hansl
Copy link
Contributor Author

hansl commented Apr 17, 2024

Users who don't want to use absolute paths on traces for security reasons.

Absolute to the root of the (very much virtual) namespace, not their filesystems. Path and FileSystem is an implementation detail. The way to go from Specifier to Path is by having a base path, like we do in SimpleModuleLoader. Everywhere else in Boa itself we would never deal with those base paths (which is the part that's sensitive).

Users who want to embed Boa in a system that doesn't have a filesystem (not possible now, WIP), but that want to expose some relative paths to the host project for debugging purposes.

This would be done using the ModuleLoader. The base would be the host project, joining specifiers with the base path when loading/resolving modules. The root of the namespace becomes the base of the host project. But that's all up to the host itself, Boa has no knowledge of these kind of details. Boa only cares about specifiers and modules.

Basically what I'm saying is:

Boa SHOULD NOT have as a concern WHERE the file is located or HOW to find it, but that there exists a map of unique Specifier => Module that can be resolved using a ModuleLoader, and that each unique absolute Specifier resolves to one and only one Module.

This is a very powerful statement, and would allow optimizations that are host independent, such as having a module cache outside of module loader, for example.

And to avoid further confusion, what I mean by "absolute Specifier" is not absolute in terms of filesystem (Boa should not be aware of filesystems or platform), but in an imaginary namespace that contains all modules handled by the host. Specifier can be seen as URIs in this context, and every module/source would have a Specifier that must be complete (not relative), and every such specifier must point to only one module.

@jedel1043
Copy link
Member

jedel1043 commented Apr 17, 2024

What would be the overall changes to Source if we go with this? Do we need to remove Source::from_filepath and ask users to open the file manually + convert their paths to Specifier beforehand?
EDIT: Also, can we offer some type of conversion from Path to Specifier?

@hansl
Copy link
Contributor Author

hansl commented Apr 17, 2024

Source::from_filepath would likely take a Specifier as an additional argument. There is nothing wrong with utility functions. We could also make it separate (a SourceExt trait for example) to keep engine clean, but I don't think that's necessary.

@hansl
Copy link
Contributor Author

hansl commented Apr 17, 2024

Also, can we offer some type of conversion from Path to Specifier?

Could be in Specifier itself. There's already a from_path, all you'd need to do is Specifier::from_path(path::strip_prefix(base))

@jedel1043
Copy link
Member

jedel1043 commented Apr 17, 2024

This discussion is getting a bit too long for a PR, and I wanted to ask some more questions about the API. I'll open a thread in Matrix.
EDIT: Link to the discussion https://matrix.to/#/!hjtiFaaZzLlPqwfAms:matrix.org/$er1XOv9CQIzd2aT2rZfiFAue4zOWBRsDx71XfcOcBtU?via=matrix.org&via=mozilla.org

@jedel1043 jedel1043 changed the title Create a new Speficier type for JavaScript module specifiers Create a new Specifier type for JavaScript module specifiers Apr 17, 2024
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.

None yet

2 participants