Enforce valid names for computed properties #1106

Rich-Harris · 2018-01-14T03:56:31Z

This is based on @asweingarten's PR #1099 (thanks!) and adds a couple of extra commits:

It makes sense to check for reserved words (like try or new) while we're doing this check
Rather than creating a fresh parser each time and wrapping it in a try-catch, it turns out we can use acorn.isIdentifierStart and acorn.isIdentifierChar to determine whether or not something is a valid identifier — same result, but more direct
Before, err.pos was pointing to the error inside the function ${name}(){} program, which doesn't correspond to the error in the component. Elsewhere, we just point to the start of the offending key, so I've done that here instead
Because this is specific to computed properties, we may as well do the check inside the existing forEach rather than iterating over the properties another time
Added a suggestion for a valid identifier in the error message (a-b-c becomes a_b_c and so on)

…rs are used in computed names Approach: For each property name, construct a string that defines a function and see if parsing that string with Acorn throws an exception. If it does, assemble an informative error message that states which property is invalid, the first invalid character, and the location of that character within the name. Changes to codebase: - Added new validator test "properties-computed-must-be-valid-function-names" - Added new check into src/validate/js/propValidators/computed.ts, "checkForValidIdentifiers" - this check was added to src/validate/js/utils/checkForValidIdentifiers.ts like the other checks in "computed.ts"

…asweingarten-master

…ted alternative

codecov-io · 2018-01-14T04:00:50Z

Codecov Report

Merging #1106 into master will increase coverage by 0.02%.
The diff coverage is 88%.

@@            Coverage Diff             @@
##           master    #1106      +/-   ##
==========================================
+ Coverage   91.56%   91.59%   +0.02%     
==========================================
  Files         123      125       +2     
  Lines        4493     4602     +109     
  Branches     1447     1514      +67     
==========================================
+ Hits         4114     4215     +101     
- Misses        162      164       +2     
- Partials      217      223       +6

Impacted Files	Coverage Δ
src/utils/fullCharCodeAt.ts	`100% <100%> (ø)`
src/validate/js/propValidators/computed.ts	`100% <100%> (ø)`	⬆️
src/parse/index.ts	`88.04% <77.77%> (-1.25%)`	⬇️
src/utils/isValidIdentifier.ts	`83.33% <83.33%> (ø)`
src/validate/html/index.ts	`96.66% <0%> (-0.71%)`	⬇️
src/generators/server-side-rendering/index.ts	`95.83% <0%> (+0.05%)`	⬆️
src/generators/Generator.ts	`94.13% <0%> (+0.25%)`	⬆️
src/validate/html/validateElement.ts	`90.74% <0%> (+0.74%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8057884...80c55b1. Read the comment docs.

Conduitry · 2018-01-14T04:31:49Z

In Parser#readIdentifier we use the regex /[a-zA-Z_$][a-zA-Z0-9_$]*/ and then check that it isn't a reserved word. Taking a quick look at Acorn's implementation, it looks like the main difference is probably that they support identifiers with non-ascii characters. Because Parser#readIdentifier uses Parser#read and because that requires a regex, we can't just use Acorn's methods there. I don't have strong opinions about whether we allow non-ascii characters in identifiers (though not allowing them sounds simpler), but I do think it should be done consistently.

emilos · 2018-01-14T09:41:20Z

src/utils/isValidIdentifier.ts

@@ -0,0 +1,11 @@
+import { isIdentifierStart, isIdentifierChar } from 'acorn';
+
+export default function isValidIdentifier(str) {


ts maybe?

export default function isValidIdentifier(str: string) : boolean {

good catch, fixed

emilos · 2018-01-14T09:42:08Z

src/validate/js/propValidators/computed.ts

+		const name = getName(computation.key);
+
+		if (!isValidIdentifier(name)) {
+			const suggestion = name.replace(/[^_$a-z0-9]/ig, '_').replace(/^\d/, '_$&');


should this be moved to utilities?

I tend to favour leaving things like this inline until you need them in more than one place, otherwise I find you can end up with a lot of premature abstractions

Rich-Harris · 2018-01-18T14:27:02Z

I've updated Parser#readIdentifier so it handles characters like 𐊧. Learned a few things in the process!

Conduitry · 2018-01-18T15:05:44Z

test/parser/samples/unusual-identifier/input.html

@@ -0,0 +1,3 @@
+{{#each things as 𐊧}}
+	<p>𐊧</p>


Did you intend for this to be <p>{{𐊧}}</p>?

* Fixing missing parentheses and using u as in doc above * added to inital app

eh-dub and others added 6 commits January 11, 2018 17:48

Merge branch 'master' of https://github.com/asweingarten/svelte into …

25a18ab

…asweingarten-master

run prettier (spaces -> tabs)

cfdc890

fix expected error position, tweak expected message to include sugges…

146327e

…ted alternative

simplify test slightly, add test for reserved words

b193036

use acorn.isIdentifierStart and isIdentifierChar to determine validity

1833bc1

Rich-Harris mentioned this pull request Jan 14, 2018

[1083] Svelte should throw a compile time error when illegal characte… #1099

Closed

Conduitry mentioned this pull request Jan 14, 2018

Component arguments that are not valid JS identifiers #887

Closed

emilos reviewed Jan 14, 2018

View reviewed changes

Rich-Harris added 2 commits January 18, 2018 08:20

types

66b64e2

handle wacky identifier names in templates

a85b09e

Conduitry reviewed Jan 18, 2018

View reviewed changes

oops

80c55b1

Conduitry approved these changes Jan 18, 2018

View reviewed changes

Rich-Harris merged commit d0be845 into master Jan 18, 2018

Rich-Harris deleted the gh-1083 branch January 18, 2018 16:48

7nik pushed a commit to 7nik/svelte that referenced this pull request Apr 9, 2025

fix: using u as in the description (sveltejs#1106)

8a182ce

* Fixing missing parentheses and using u as in doc above * added to inital app

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce valid names for computed properties #1106

Enforce valid names for computed properties #1106

Rich-Harris commented Jan 14, 2018

codecov-io commented Jan 14, 2018 •

edited

Loading

Conduitry commented Jan 14, 2018

emilos Jan 14, 2018

Rich-Harris Jan 18, 2018

emilos Jan 14, 2018 •

edited

Loading

Rich-Harris Jan 18, 2018

Rich-Harris commented Jan 18, 2018

Conduitry Jan 18, 2018

Rich-Harris Jan 18, 2018

		@@ -0,0 +1,11 @@
		import { isIdentifierStart, isIdentifierChar } from 'acorn';

		export default function isValidIdentifier(str) {

Enforce valid names for computed properties #1106

Enforce valid names for computed properties #1106

Conversation

Rich-Harris commented Jan 14, 2018

codecov-io commented Jan 14, 2018 • edited Loading

Codecov Report

Conduitry commented Jan 14, 2018

emilos Jan 14, 2018

Choose a reason for hiding this comment

Rich-Harris Jan 18, 2018

Choose a reason for hiding this comment

emilos Jan 14, 2018 • edited Loading

Choose a reason for hiding this comment

Rich-Harris Jan 18, 2018

Choose a reason for hiding this comment

Rich-Harris commented Jan 18, 2018

Conduitry Jan 18, 2018

Choose a reason for hiding this comment

Rich-Harris Jan 18, 2018

Choose a reason for hiding this comment

codecov-io commented Jan 14, 2018 •

edited

Loading

emilos Jan 14, 2018 •

edited

Loading