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

[feat] Implement RFC 33 - Constants in markup #6413

Merged

Conversation

tanhauhau
Copy link
Member

Implementing sveltejs/rfcs#33

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with npm test and lint the project with npm run lint

@tanhauhau tanhauhau changed the title implement rfc 33 Implement RFC 33 - Constants in markup Jun 20, 2021
@tanhauhau tanhauhau force-pushed the tanhauhau/rfc-33-constants-in-markup branch from 70e4128 to c534da3 Compare June 20, 2021 12:22
@tanhauhau
Copy link
Member Author

@benmccann hmm.. but the CI / Lint didnt complain about them. i wonder should we have a separate PR that lints .svelte files in the test folder and run it through all the files at once?

@benmccann
Copy link
Member

I'm not sure it'd be possible to lint the .svelte files in the tests because so many of them are purposefully malformed, etc. to test error handling. I don't think we have to be a stickler for .svelte file formatting in tests, but I just saw a number of files where spaces and tabs were mixed, which makes it harder to read and thought we could at least be consistent on that

@tanhauhau tanhauhau force-pushed the tanhauhau/rfc-33-constants-in-markup branch from c5dd882 to 42652bb Compare June 27, 2021 03:20
@benmccann benmccann changed the title Implement RFC 33 - Constants in markup [feat] Implement RFC 33 - Constants in markup Jun 30, 2021
@tanhauhau tanhauhau force-pushed the tanhauhau/rfc-33-constants-in-markup branch from 42652bb to 2bbc535 Compare July 24, 2021 04:51
@bluwy bluwy mentioned this pull request Aug 2, 2021
@Conduitry
Copy link
Member

The test failures here look to be genuine, and appear to be related to TypeScript complaining about types.

@Conduitry Conduitry marked this pull request as draft August 4, 2021 16:24
@iamcalvintam
Copy link

Hi guys, what would be the showstopper here? This looks wonderful and I personally couldn't wait to use it.

@tanhauhau tanhauhau force-pushed the tanhauhau/rfc-33-constants-in-markup branch from 2bbc535 to 78fe8a2 Compare September 26, 2021 07:05
@tanhauhau tanhauhau marked this pull request as ready for review September 26, 2021 07:15
@Conduitry
Copy link
Member

I've just pushed a change to update the wording of a couple of compiler error messages.

@tanhauhau Is there a reason for only allowing {@const} inside the specific node types that you currently are?

In particular, why is {#await foo} {@const x = y} {/await} prohibited? What about the {:else} of an {#each}? In some of Rich's comments on the original RFC, he also used {@const} inside {#if}, which is also disallowed by this implementation. Were you trying to avoid having {@const} in places that weren't already creating their own new scope?

@iamcalvintam
Copy link

Based on my use cases for const, most of the time it would work best in an #each loop. But I believe it would be as useful in an #await as well to transform the returned response. Would be interested to hear your opinions, @tanhauhau . Also thanks so much for working on this PR.

@iamcalvintam
Copy link

Looks like the work is completed here guys? When are we expecting this to be merged

@tanhauhau tanhauhau force-pushed the tanhauhau/rfc-33-constants-in-markup branch from 00a4a9b to 79063bd Compare November 27, 2021 16:39
@iamcalvintam
Copy link

Thanks so much for the great work @tanhauhau !

@Theo-Steiner
Copy link
Contributor

Wow, terrific work @tanhauhau!
I wonder if this should be included in the tutorial for each blocks?
We could modify the cat example by giving the cats a weight property but the property is in pounds (because an evil american made the cat list) so now the user has to make a @const to help non-imperial unit folks understand the size of the cats as well.

Task:

<script>
	let cats = [
		{ id: 'J---aiyznGQ', name: 'Keyboard Cat', weight: 8},
		{ id: 'z_AbfPXTKms', name: 'Maru', weight: 11 },
		{ id: 'OUtn3pvWmpg', name: 'Henri The Existential Cat' , weight: 15}
	];
</script>

<h1>The Famous Cats of YouTube</h1>

<ul>
	<!-- open each block -->
                 <!-- declare constant -->
		<li><a target="_blank" href="https://www.youtube.com/watch?v={cat.id}">
			{cat.name} is a healthy {catWeight} kg!
		</a></li>
	<!-- close each block -->
</ul> 

Solution:

<script>
	let cats = [
		{ id: 'J---aiyznGQ', name: 'Keyboard Cat', weight: 8},
		{ id: 'z_AbfPXTKms', name: 'Maru', weight: 11 },
		{ id: 'OUtn3pvWmpg', name: 'Henri The Existential Cat' , weight: 15}
	];
</script>

<h1>The Famous Cats of YouTube</h1>

<ul>
	{#each cats as { id, name, weight }, i}
		<li><a target="_blank" href="https://www.youtube.com/watch?v={id}">
			{@const catWeight = weight /  2.205}
			{i + 1}: {name} is a healthy {catWeight} kg!
		</a></li>
	{/each}
</ul>

Maybe {@const } should get a whole tutorial section of its own though?

@SarcevicAntonio
Copy link
Member

@Theo-Steiner imo every tag should be introduced one at a time as to not overwhelm beginners.

@aradalvand
Copy link

aradalvand commented Dec 23, 2021

Personally, I don't like the keyword const for this, I think {@value ...} or {@local ...} were actually more fitting, which were the alternatives suggested in the original RFC too — even something like {$: ...} to match the reactive declaration syntax, since these are really local reactive variables — the reason for this is because a {@const ...} is not really the same thing as a JavaScript constant but the keyword makes it look like it is, while it's essentially a Svelte-specific construct, plus there's also no {@let ...} or {@var ...} counterpart, unlike what one might intuitively guess, so I think it can be confusing.

@tanhauhau tanhauhau force-pushed the tanhauhau/rfc-33-constants-in-markup branch from 7c6baff to 965669f Compare January 8, 2022 16:18
@Conduitry Conduitry force-pushed the tanhauhau/rfc-33-constants-in-markup branch from 854cc3d to bb5ad3b Compare January 11, 2022 21:56
@janosh
Copy link
Contributor

janosh commented Jan 11, 2022

Poor Constantine Plotnikov is being pinged to no end from this PR. 😄

@Conduitry
Copy link
Member

🤷 I tried putting the reference in the commit message in backticks, but it doesn't seem like that prevents the ping there. Oh well!

@Conduitry Conduitry merged commit b5aaa66 into sveltejs:master Jan 11, 2022
@tanhauhau tanhauhau deleted the tanhauhau/rfc-33-constants-in-markup branch January 11, 2022 23:57
@lukaszpolowczyk
Copy link

Wait, why doesn't it work in if else? :O

@opensas
Copy link
Contributor

opensas commented Feb 8, 2022

@tanhauhau Is there a reason for only allowing {@const} inside the specific node types that you currently are?

I also wonder it there's any reason why the @const tag couldn't be supported as a direct child of an {#if...} tag. I think it would be pretty useful. May be I'm missing something.

(I'm not sure if this is the right place to post this question, given that the PR is already closed...)

@julien-c
Copy link

I also wonder it there's any reason why the @const tag couldn't be supported as a direct child of an {#if...} tag.

It is now though, no?

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.