withastro / roadmap

Ideas, suggestions, and formal RFC proposals for the Astro project.
292 stars 29 forks source link

Proposal: Support a `local:` attribute prefix #59

Closed jonathantneal closed 2 years ago

jonathantneal commented 2 years ago

Summary

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

Links

matthewp commented 2 years ago
sgruenholz2 commented 2 years ago

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

natemoo-re commented 2 years ago

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 commented 2 years ago

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 './my-kitten@2x.avif' 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 commented 2 years ago

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 commented 2 years ago

This RFC will be updated with the following changes:

FredKSchott commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

<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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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


Platform Import Resolution

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

Platform URL Resolution

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


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:

jonathantneal commented 2 years ago

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

altano commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 astro@0.22.20):

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">