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

fix(compiler): only transform relative asset URLs #628

Merged
merged 2 commits into from
Jan 20, 2020

Conversation

sisou
Copy link
Contributor

@sisou sisou commented Jan 16, 2020

Issue first reported in vue-loader: vuejs/vue-loader#1632

Absolute asset URLs, such as those that start with a forward-slash / or are even http(s):// URLs are currently interpreted as relative URLs and their path is transformed into an import expression by the SFC compiler.

This PR adds a utility method isRelativeUrl and uses it to differentiate between URLs that need to be imported and thus transformed, and those that do not.

I can also imagine other names for the helper method, such as requiresTransform.

I only started looking into the source yesterday, so if the solution presented in this PR does not match with how you would solve this, feel free to give pointers and close this.

context
)
let exp: ExpressionNode
if (isRelativeUrl(attr.value.content)) {
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 we should simply return early if the URL is absolute. This keeps src a static attribute and allows the entire img element to be hoisted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, this didn't even occur to me to simply return. Changed it here accordingly and reverted the condition brackets.

@yyx990803
Copy link
Member

👍

@sisou sisou deleted the sisou/absolute-assets branch February 23, 2021 14:14
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.

3 participants