search

web-infra-dev / rspack

The fast Rust-based web bundler with webpack-compatible API 🦀️
https://rspack.dev
MIT License
10.25k stars 579 forks source link

[Tracking]: Improve dependency structure specification #7422

Open shulaoda opened 4 months ago

shulaoda commented 4 months ago

What problem does this feature solve?

Among many dependencies, they mark the scope of the dependency as span, range, or start&end. This behavior is very chaotic in Dependency.

https://github.com/web-infra-dev/rspack/blob/f37741c316c30e6234cfc504efbabea53a26df75/crates/rspack_plugin_javascript/src/dependency/esm/harmony_export_header_dependency.rs#L11-L16

https://github.com/web-infra-dev/rspack/blob/f37741c316c30e6234cfc504efbabea53a26df75/crates/rspack_plugin_javascript/src/dependency/esm/harmony_import_dependency.rs#L43-L54

https://github.com/web-infra-dev/rspack/blob/f37741c316c30e6234cfc504efbabea53a26df75/crates/rspack_core/src/dependency/cached_const_dependency.rs#L7-L12

This 0-based range is based on the 1-based SWC's Span conversion, and I think we should name it range which used in webpack to avoid ambiguity.

In addition, I think it may be better to name the types of loc and range as DependencyLocation and DependencyRange.

What's your opinion? @h-a-n-a

What does the proposed API of configuration look like?

pub struct Dependency {
  loc: DependencyLocation,
  range: DependencyRange
}
### Tasks
- [ ] https://github.com/web-infra-dev/rspack/issues/7336
- [ ] https://github.com/web-infra-dev/rspack/pull/7576
- [ ] https://github.com/web-infra-dev/rspack/pull/7635
- [ ] https://github.com/web-infra-dev/rspack/pull/7641
- [ ] https://github.com/web-infra-dev/rspack/pull/7671
- [ ] https://github.com/web-infra-dev/rspack/pull/7841
- [ ] https://github.com/web-infra-dev/rspack/pull/7871
- [ ] https://github.com/web-infra-dev/rspack/pull/7892
h-a-n-a commented 4 months ago

Thanks for filing this issue! The dependency struts are indeed chaotic and a messed-up.

First, for the status-quo, we do over-used the word Span. To me, Span is more likely to be a specific word using to describe the internal data of a compiler(SWC in this case) and if this was to used in other cases, it will definitely mess up the definition.

Second, I like the idea of using DependencyRange to describe the 0-based range and the idea of DependencyLocation. On top of this, I would love them to be derived from Span(exposing fn to_loc and fn to_range out). This would not likely to cause any mistakes and avoid adding to much methods to module traits.

If the current impl cannot meet your needs, you may create your own RspackSpan to iron out these problems.

shulaoda commented 4 months ago

This issue is related to work #7336 , and I can try to address it.

In addition, it is worth mentioning that some dependencies related to CSS do not come from Span, but from simple Range. Perhaps we can also try using Trait From.

h-a-n-a commented 4 months ago

This issue is related to work #7336 , and I can try to address it.

In addition, it is worth mentioning that some dependencies related to CSS do not come from Span, but from simple Range. Perhaps we can also try using Trait From.

Then I guess a custom RspackSpan would be enough to describe them all. We can implement trait From to all of the raw Spans provided by compilers.

ahabhgk commented 2 months ago

The work on DependencyLocation is a great job, but we can improve some types to make the semantics more strict, for example: BasicEvaluatedExpression::with_range

The type of BasicEvaluatedExpression::with_range is https://github.com/web-infra-dev/rspack/blob/0969538afd86f58c6cc64eeb855c8985bdaf2429/crates/rspack_plugin_javascript/src/utils/eval/mod.rs#L126

it accepts two u32 to create an RealDependencyLocation (0-based), but the usage of it is accepting start and end from Span (1-based), start is changed to 0-based by real_lo, but end didn't, this is actually different with the semantics of RealDependencyLocation (start and end are both 0-based)

https://github.com/web-infra-dev/rspack/blob/0969538afd86f58c6cc64eeb855c8985bdaf2429/crates/rspack_plugin_javascript/src/utils/eval/eval_array_expr.rs#L24

so for this case we can improve the type to something like this, to strictly enforce the semantics of RealDependencyLocation: start and end are both 0-based

pub fn with_range(location: impl IntoDependencyLocation) -> Self

impl IntoDependencyLocation for Span {
  // change to 0-based
}

The basic idea is avoid using (u32, u32) directly as much as possible, all RealDependencyLocation is transformed from another type (swc::Span, css_module_lexer::Range, etc)

stale[bot] commented 3 weeks ago

This issue has been automatically marked as stale because it has not had recent activity. If this issue is still affecting you, please leave any comment (for example, "bump"). We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!