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

Proposal: Support a local: attribute prefix #59

Merged
merged 4 commits into from
Jan 5, 2022

Conversation

jonathantneal
Copy link
Contributor

  • Start Date: 2021-12-20
  • Status: Draft

Summary

This proposal seeks to improve source-relative URL support in Astro files.

Links

@jonathantneal jonathantneal self-assigned this Dec 20, 2021
@matthewp
Copy link
Contributor

  • I like the change to a URL scheme rather than an attibute value prefix because it allows the user the flexibility to use this anywhere inside of any attribute value.
  • However this might go against the idea that Astro doesn't touch your code unless you use a directive. What if the user intended the local: string to be inside of an attribute value?
    • I think it's still worth doing, just calling out that it contradicts with what we had been saying we would do.
    • This concern could be alleviated by allowing a way to escape local: when it's intended to be used as a string and not a URL scheme.

@sgruenholz2
Copy link
Contributor

I may be missing some context, but coming in fresh to Astro here are my initial thoughts/expectations:

  • I expect anything in an image src to always be relative to the static compiled html and where the page is compared to the public dir. So, in practice, I would never use something like <img src="kitten.avif" alt="Kitten" />. I would only ever use <img src="/kitten.avif" alt="Kitten" /> or <img src="{my_cdn_host}/kitten.avif" alt="Kitten" /> or roll my own custom src helper function if I need to conditionally rebase.
  • If you need a way to determine which src image files should be copied into the public dir, I would expect to set that up as part of the config via glob patterns, or sure, do that automatically if an import is present in the frontmatter that then gets referenced as a src in the html, or if there's an Astro.resolve() helper present. Or you could have a standard src/assets dir where everything in there always gets copied into the public dir.
  • It feels like there's probably a better approach to solve the underlying problems here.
  • If we do go with a local: prefix as part of the src value, then yes, I'd expect it to also work for styles.

@natemoo-re
Copy link
Member

natemoo-re commented Dec 20, 2021

Even though we should prioritize DX over what is easier to implement, I think it’s worth considering that there isn’t a mechanism to support a feature like this in Rollup/Vite at the moment and we’d be forging our own path here.

I have open questions about how we could handle this URL scheme in CSS, things like the style property (especially custom properties), and if we support this in non-Astro files.

I would have a lot fewer questions and concerns with a local: prefixed attribute since I have a clear understanding of the boundaries that sets for us.

@jonathantneal
Copy link
Contributor Author

jonathantneal commented Dec 20, 2021

Hey @sgruenholz2, I appreciate the feedback. I’ll do my best to address your points.

I expect anything in an image src to always be relative to the static compiled html.

I feel the same way. I believe others do, too. However, after many conversations I’m convinced we are, at the very least, not a super-majority. 😅

... you could have a standard src/assets dir where everything in there always gets copied into the public dir.

Well, that kinda seems like another public directory. What we’ve seen before Astro and now within Astro is that developers continually reject separation by type and prefer colocation by context. They want to keep files close together that go together and change together. It’s the "colocation principle" 1.

It feels like there's probably a better approach to solve the underlying problems here.

The prior approach has been Astro.resolve, and this approach has been problematic. I say that as someone who helped implement it. Our repo has quite a few "relative" and "resolve" issues 1, and I think the same is true within Discord. I’m very okay with doing something other than Astro.resolve now. I like that folks like Astro best when they can avoid JavaScript expressions. 😄

In a prior proposal I did suggest something like this:

---
import Kitten1x from './my-kitten.avif' assert { type: "url" }
import Kitten2x from './[email protected]' assert { type: "url" }
import linkToScript from './script.js' assert { type: "url" }
---

However, there was still a significant desire to keep links within HTML, which in fairness is how HTML intended it 1. I also bring up "fatigue" in this RFC, because import statements introduce this by requiring authors to come up with names for any asset they’ll want to use, and possibly yet another naming scheme for assets in general.

@FredKSchott
Copy link
Member

Love the exploration happening here, but agreed with other comments here that this takes a step away from previous iterations of this feature in ways that are definitely concerning from a complexity perspective. I'm a bigger fan of the smaller-scoped, more explicit local:src directive that was proposed in the last iteration.

@jonathantneal
Copy link
Contributor Author

This RFC will be updated with the following changes:

  • Use the alternative design (local: attribute prefix).
  • Ensure the following details are already present (otherwise added) from the call:
    • local: attribute prefix treats the entire attribute value as the URL.
    • local: attribute prefix works on any attribute.
      • limiting the attribute prefix to one attribute name or element type would add complexity.
    • ?url is presumed to be appended to the value.
    • attribute values with partial sources (srcset, style, some data attributes) are not handled, but can be handled in a separate, future RFC.

@jonathantneal jonathantneal changed the title Add Relative URL Scheme proposal Proposal: Support a local: attribute prefix Jan 4, 2022
@FredKSchott
Copy link
Member

FredKSchott commented Jan 4, 2022

LGTM based on notes! For the record books: I'd still love to bikeshed the final client directive name in the implementation RFC (slight preference for import:src or resolve:src) but that is in no way blocking this RFC.

Also: is there any reason that you're saying "attribute prefix" instead of "client directive"? Do we need a new term for client directives that can dynamically be applied to any local:* attribute? Mostly curious, not blocking.

@jonathantneal
Copy link
Contributor Author

jonathantneal commented Jan 4, 2022

slight preference for import:src or resolve:src

The name local was chosen to avoid confusion with 'import' resolution in JS. The name import or resolve may suggest it works like ESM import or CJS resolve. We experienced this issue previously, due to the naming of Astro.resolve.

We didn’t spend much time discussing how the URL is transformed on the call (outside that it would append ?url), and we didn’t specifically discuss what kinds of URLs are allowed. The examples demonstrate the intended resolution, which is local only, but it’s not actually written down. It should be written down, unless we each think it should work differently.

The intended behavior is to match how one would normally reference the value, so that src="kitten.avif" would become local:src="kitten.avif".

If the name were import or resolve, we might see authors writing something like this and expecting it to work:

<!-- THIS WON’T WORK -->
<link resolve:href="@csstools/normalize.css" type="stylesheet" />

I realize this wasn’t already written down because the earlier proposal made the scheme very clear, sort of like node:fs.

I also realize the lack of scheme may introduce other issues, since it may be unclear to authors that the magic doesn’t include us covering for things some might consider doing the right thing.

<!-- THIS WON’T WORK -->
<link import:href="https://unpkg.com/@csstools/normalize.css" type="stylesheet" />

@jonathantneal
Copy link
Contributor Author

Also: is there any reason that you're saying "attribute prefix" instead of "client directive"? Do we need a new term for client directives that can dynamically be applied to any local:* attribute?

If we wanted, we could refer to them as local directives. Thoughts? If there’s consensus, I can use that term. We have a class:list directive. I wouldn’t recommend client directives, since I expect those are focused on a set of features implemented with a client: attribute prefix.

@FredKSchott
Copy link
Member

<link resolve:href="normalize-css" type="stylesheet" />

Let me think about this a bit more, but I'm not sure I agree on the risk vs. reward here. Because we're using import() in our implementation, talking about this as sugar for import(), and would most likely document this in our docs by talking about import(), I think that the import/resolve connection might be a feature and not a bug.

import:src or resolve:src both communicate this more clearly to a user in a self-documenting way. With local:src, as a user I'm definitely going to think "wait, what does local mean again?" at least a few times. At least, I've never made that connection for the meaning of local until you just explained it to me.

@jonathantneal
Copy link
Contributor Author

jonathantneal commented Jan 4, 2022

I think that the import/resolve connection might be a feature and not a bug.

To be certain, would you expect this feature to resolve node modules?

Currently, as this transformation is static, @csstools/normalize.css would become ./@csstools/normalize.css?url, which would look for a local file relative to the source.

And for a reference with a leading ./, ./kitten.avif would become ././kitten.avif?url, where the ././ would still resolve correctly.

@aFuzzyBear
Copy link

aFuzzyBear commented Jan 5, 2022

Hi guys, I hope you dont mind me commenting on this, I have bee giving this a fair bit of thought since the call yesterday.
Reading the comments above, Im very much a proponent for this rfc, its spirit and goals are very much aligned to providing an easier syntax to help resolve files from the ./src/
If we are discussing the bikeshedding merits of local: I would like to express my own thoughts on this,
The use of the term local: confers its purpose very well, applied as a attribute directive, *Tangentially, I feel that it was inevitable that the astro lexicon grew to encompass more terms, "client directive": client: has more application and easier separation if there was a counteract or balance to it, with use of attribute directives, local:src being an example.
Local is like a "Ronsil" description in my opinion; Ronsil for those unfamiliar was a paint manufacturer whose marketing strapline was "It does what it says on the Tin".
This sort of simple self explanatory application of adverbs and pronouns must be central to the debate over bikeshedding terms,
There was however a contention that comes from the actual use of the term local:, principally its meant to replace the use of Astro.resolve() with a more static, syntactically friendly attribute, that would help both compiler and developer. If this is the case, then in equal measure we must consider the term resolve:src or resolve:href to its contemporary counterpart, local:src local:href .

I must confess I am torn between the two, its not often I find myself being ambivalent about things, I do not wish to be the one who makes the final decision on this, 😅

@jonathantneal
Copy link
Contributor Author

We may not have expected this feature does the same thing, but I think we can reach consensus on this issue concerning resolution.

I recommend that we embrace web platform resolution in Astro HTML, perhaps resembling Deno. I recommend that we do not embrace CommonJS resolution.

CommonJS Resolution

In NodeJS, require and import statements require a leading ./, and are otherwise considered CommonJS modules. CommonJS modules (like @csstools/normalize.css) are resolved against a lengthy algorithm. ref

  • ComonJS resolution is not on any standards track.
  • ComonJS resolution has bad performance. ref
  • For backwards compatibility, NodeJS supports ComonJS resolution in import statements (and dynamic imports).

Platform Import Resolution

In JS, import statements (and dynamic imports) require a leading ./ or an absolute URL.

  • There is a Import Maps proposal, which would allow imports similar to CommonJS. ref
  • Import Maps are on a standards track. ref
  • Import Maps and absolute URLs are supported in Deno. ref
  • Import Maps and absolute URLs are not supported (out-of-the-box) in NodeJS.
  • Import Maps resolve differently than CommonJS.

Platform URL Resolution

In HTML src, href, etc. & JS location, URL; relative URLs do not require a leading ./.

  • Example, given a location of https://example.com/cats/:
    • In HTML, <img src="assets/kitten.avif" /> resolves to https://example.com/cats/assets/kitten.avif.
    • In JS, new URL("assets/kitten.avif", location) resolves to https://example.com/cats/assets/kitten.avif.
  • This is very convenient for sibling URLs.

As Astro blends server-side and client-side functionality, we are caught between these worlds, and others. For example, we support (thru Vite) URLs that are relative to the source file, or relative to the project, or relative to the public directory. Thru Vite and TypeScript, we have also supported aliased URLs.

Do we need a resolution unification RFC?

I would love to see Astro stick with web platform resolution, except when aliasing is explicitly defined by the project. These would be:

  • Import Maps (astro.config.js#config.imports)
  • Defined dependencies (package.json#dependencies)
  • TypeScript aliases (tsconfig.json#compilerOptions.paths, jsconfig.json#compilerOptions.paths)

@jonathantneal
Copy link
Contributor Author

After some discussion, we’ll merge this as-is and leave the details to the point of implementation.

@jonathantneal jonathantneal merged commit 021e99d into main Jan 5, 2022
@jonathantneal jonathantneal deleted the jn.relative-url-scheme-proposal branch January 5, 2022 17:26
@altano
Copy link

altano commented Feb 15, 2022

I don't know if I'm too late to the party, but I believe this RFC doesn't address Markdown since there are no attributes to prefix with local:. Is that intended?

The main use case I'm interested in is having a colocated image referenced from Markdown, e.g.:

![Save](./save.png)

I feel the same way. I believe others do, too. However, after many conversations I’m convinced we are, at the very least, not a super-majority. 😅

If you have a moment could you provide any pointers, @jonathantneal? I'm having a little trouble finding prior discussion of this and I'm having trouble imagining that anyone likes the current behavior (especially in Markdown)?

@jonathantneal
Copy link
Contributor Author

jonathantneal commented Feb 15, 2022

I don’t have pointers, because I believe this RFC is insufficient for Markdown. I believe we should support ![alt](src) and <img src> as-is. My opinions about resolution changed, but so did the RFC before it was accepted. I did not want local:src as an attribute name in the original proposal, which makes offering pointers for it particularly challenging. As for supporting <img src>, I am grateful that our community and users convinced me, and I hope changing my mind does not give you a lack of faith in me or the process.

To be specific, I think we should support <img src> and ![alt](src) out of the box, as long as scoping is restricted to the project, and root slash is relative to the project root. I think unmatched paths can ship as-is (allowing them to behave as this RFC intended, from public). For Markdown files, this means a /docs/file.md with either ![Save](save.png) or ![Save](./save.png) or ![Save](/docs/save.png) should just work™. I believe all 3 of those examples will currently work in GitHub, too.

And to highlight the restriction, I reject Vite’s current behavior where /Users/your_name/GitHub/project/docs/file.html can have <img src="/Users/your_name/Pictures/private.jpg" /> and Vite is like "yea sure no problem lets add that to the asset graph" without requiring any special user permission/flag. I’m not sure if we can disable this, or if the change needs to happen upstream, but that is my only remaining hesitation doing things the 'Vite' way. 😅

@altano
Copy link

altano commented Feb 15, 2022

I am grateful that our community and users convinced me, and I hope changing my mind does not give you a lack of faith in me or the process.

Astro is great (especially given how young the project is) and I have nothing but faith in the process thus far :). I am a little unclear on what you were convinced of and what the future direction is. Is it the merged rfc?

For Markdown files, this means a /docs/file.md with either ![Save](save.png) or ![Save](./save.png) or ![Save](/docs/save.png) should just work™. I believe all 3 of those examples will currently work in GitHub, too.

Yeah, that sounds good. I don't even particularly care how we get there or what it looks like exactly as long as the goal of colocation is achieved. Splitting resources between public/ and src/ (and replicating directory structure, and hard-coding paths that might break) just feels not super great. Although, I wouldn't even really consider it a dealbreaker.

FWIW the biggest head scratcher with the current behavior is how unpredictable the behavior is. This code:

~/src/pages/articles/my-article/index.md:

---
setup: |
  import saveReference from './save.png'
---

![Save](./save.png)

<img src={saveReference} />

Gives you (in [email protected]):

specification astro dev astro build && astro preview
![Save](./save.png) <img src="./save.png" alt="Save"> <img src="./save.png" alt="Save">
<img src={saveReference} /> <img src="../../assets/save.32416143.png"> <img src="/src/pages/articles/my-article/save.png">

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.

7 participants