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

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

Closed
wants to merge 1 commit into from

Conversation

eh-dub
Copy link
Contributor

@eh-dub eh-dub commented Jan 12, 2018

…rs are used in computed names

Problem #1083 :
Computed property names are naively transformed into function identifiers so random-api becomes random-api(){} which is not allowed.

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"

…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"
@codecov-io
Copy link

codecov-io commented Jan 12, 2018

Codecov Report

Merging #1099 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1099      +/-   ##
==========================================
+ Coverage   91.56%   91.57%   +0.01%     
==========================================
  Files         123      124       +1     
  Lines        4493     4501       +8     
  Branches     1447     1447              
==========================================
+ Hits         4114     4122       +8     
  Misses        162      162              
  Partials      217      217
Impacted Files Coverage Δ
src/validate/js/utils/checkForValidIdentifiers.ts 100% <100%> (ø)
src/validate/js/propValidators/computed.ts 100% <100%> (ø) ⬆️

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 2537db9...02afdb0. Read the comment docs.

@Rich-Harris
Copy link
Member

Thank you! There were a couple of small tweaks I needed to make, and the easiest thing was to open a fresh PR on top of this one, so I've done that — closing in favour of #1106

@eh-dub
Copy link
Contributor Author

eh-dub commented Jan 14, 2018

Awesome, glad to have been able to kick things off.

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