-
-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat: add TransformAssetUrls options #64
base: master
Are you sure you want to change the base?
Conversation
@@ -0,0 +1,51 @@ | |||
use std::collections::HashMap; |
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.
fervid_core
has access to fxhash
, can you use it instead?
pub fn with_bindings_helper( | ||
bindings_helper: BindingsHelper, | ||
transform_asset_urls: Option<TransformAssetUrls>, | ||
) -> CodegenContext { |
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.
upd: As said in other comments, transform_asset_urls
should really be in the fervid_transform
, please revert this change
I don't really like the idea of forcing transform_asset_urls
into the constructor.
Maybe the constructor should be renamed then to be more intuitive?
CoreTransformAssetUrls::Options(CoreAssetURLOptions { | ||
base: value.base.clone(), | ||
include_absolute: value.include_absolute.unwrap_or(false), | ||
tags: value.tags.clone(), |
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 converting from std::collections::HashMap
to fxhash::FxHashMap
should be rather trivial
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.
You should prefer using structs.rs
for fervid_napi
to keep configuration close as of now. Fervid is already huge and having lots of small files does not help with navigation 🙁
/// Whether to process absolute URLs | ||
pub include_absolute: bool, | ||
/// Tag-specific configuration | ||
pub tags: Option<AssetURLTagConfig>, |
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 needs to be non-option, because it is fervid_core
.
The assumption is: fervid_core
gets "clean" structures into it. Otherwise every crate which depends on it will need to unwrap
with some kind of default values. fervid_core
can provide defaults as you already did.
pub tags: Option<AssetURLTagConfig>, | |
pub tags: AssetURLTagConfig, |
#[derive(Debug, Clone)] | ||
pub enum TransformAssetUrls { | ||
// TODO | ||
// export interface AssetURLTagConfig { | ||
// [name: string]: string[] | ||
// } | ||
Boolean(bool), | ||
Options(AssetURLOptions), | ||
} |
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.
Same comment here, do not wrap the option in an enum, use AssetURLOptions
directly.
You can do wrapping in the "external" crates, such as fervid_napi
, fervid_wasm
and fervid_farmfe
because there you need freedom
#[derive(Debug, Clone)] | |
pub enum TransformAssetUrls { | |
// TODO | |
// export interface AssetURLTagConfig { | |
// [name: string]: string[] | |
// } | |
Boolean(bool), | |
Options(AssetURLOptions), | |
} |
#[napi(object)] | ||
#[derive(Clone, Debug)] | ||
pub struct NapiAssetUrlOptions { | ||
pub base: Option<String>, | ||
pub include_absolute: Option<bool>, | ||
pub tags: Option<HashMap<String, Vec<String>>>, | ||
} |
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.
You can in theory accept an enum Options, Boolean
here, but let's not do it now and stay at your current code
CoreTransformAssetUrls::Options(CoreAssetURLOptions { | ||
base: value.base, | ||
include_absolute: value.include_absolute.unwrap_or(false), | ||
tags: value.tags, | ||
}) |
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.
CoreTransformAssetUrls::Options(CoreAssetURLOptions { | |
base: value.base, | |
include_absolute: value.include_absolute.unwrap_or(false), | |
tags: value.tags, | |
}) | |
CoreAssetURLOptions { | |
base: value.base, | |
include_absolute: value.include_absolute.unwrap_or(false), | |
tags: value.tags, | |
} |
// TODO Url processing based on different tags is performed. Currently, src is processed as a whole first | ||
/** | ||
* const defaultAssetUrlOptions = { | ||
* base: null, | ||
* includeAbsolute: false, | ||
* tags: { | ||
video: ['src', 'poster'], | ||
source: ['src'], | ||
img: ['src'], | ||
image: ['xlink:href', 'href'], | ||
* use: ['xlink:href', 'href'], | ||
* }, | ||
* } | ||
*/ |
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.
This looks very much like a template
transform phase instead of a codegen phase. You also asked a question about where to put the asset url code so that it fits.
Here's the general concept:
transform
phase does AST manipulations, for both<script>
and<template>
. It means it can change anything on the AST, includingsrc: '~/foo/bar'
->src: _imported
. You would do this by changing fromAttributeOrBinding::RegularAttribute
intoAttributeOrBinding::VBind
(what you are trying to do, essentially). With the change,codegen
will now correctly generate theExpr
you put intoVBind
instead of a string forRegularAttribute
.codegen
phase does not modify the AST, but instead builds its ownModule
:fervid/crates/fervid_codegen/src/control_flow/sfc.rs
Lines 46 to 53 in 7403daf
pub fn generate_module( &mut self, template_expr: Option<Expr>, mut script: Module, mut sfc_export_obj: ObjectLit, mut synthetic_setup_fn: Option<Box<Function>>, gen_default_as: Option<&str>, ) -> Module {
So the easiest for you is to implement another transform for asset URLs inside fervid_transform
.
I am also very sorry if the structure is a bit messy, I am ideating about the new fervid_transform
structure for quite a while now, but this part is very untrivial. @vue/compiler-sfc
does a lot of strange things which Rust simply does not allow, for example as any
casts or having a parentStack
.
hash: Option<String>, | ||
} | ||
|
||
impl ParsedUrl { |
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.
This 100% belongs into its own transform
@ErKeLost Here is an example of a remap from fervid/crates/fervid_transform/src/template/ast_transform.rs Lines 536 to 546 in 7403daf
If you need any more assistance, do not hesitate to write 🙂 |
Thanks for your review, I will try to modify the code according to your method. You suggested that I should make the modification attributes in transform instead of codegen, right? Let me look at the code in the transform first |
close #63
@phoenix-ru PTAL
I have temporarily solved the problem of processing assets url paths in the development environment. Next, I will sort out all the attribute-related code, which can also help me better understand the code. Next, I will realize the splitting of the overall attribute structure.