Skip to content

Fix #7590: Allow 'readonly' to be used in constructor parameters #8555

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.

Sign up for GitHub

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

Jump to bottom
Merged
6 commits merged into from
May 12, 2016
Merged

Fix #7590: Allow 'readonly' to be used in constructor parameters #8555

6 commits merged into from
May 12, 2016

Conversation

ghost
Copy link

@ghost ghost commented May 11, 2016

Fixes #7590

@msftclas
Copy link

Hi @Andy-MS, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!


It looks like you're a Microsoft contributor (Andy Hanson). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

DanielRosenwasser
DanielRosenwasser reviewed May 11, 2016
View reviewed changes
src/compiler/types.ts
AccessibilityModifier = Public | Private | Protected,
// Accessibility modifiers and 'readonly' can be attached to a parameter in a constructor to make it a property.
Copy link
Member

@DanielRosenwasser DanielRosenwasser May 11, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The term for this is a "parameter property", so maybe a better name for the flag would be ParameterPropertyModifier

yuit
yuit reviewed May 11, 2016
View reviewed changes
src/compiler/types.ts
@@ -407,8 +407,10 @@ namespace ts {
HasAggregatedChildData = 1 << 29, // If we've computed data from children and cached it in this node
HasJsxSpreadAttribute = 1 << 30,

Modifier = Export | Ambient | Public | Private | Protected | Static | Abstract | Default | Async,
Modifier = Export | Ambient | Public | Private | Protected | Static | Abstract | Default | Async | Readonly,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you still want to keep Readonly in ? since you have made a new flag ParameterPropertyModifier

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is a modifier..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got cha, I got confused why it is here caz i thought it is not direct related with the bug.

@yuit
Copy link
Contributor

yuit commented May 11, 2016

I believe that TypeScript Spec should be updated to include this change

yuit
yuit reviewed May 11, 2016
View reviewed changes
...nce/classes/constructorDeclarations/constructorParameters/readonlyInConstructorParameters.ts
@@ -0,0 +1,13 @@
class C {
constructor(readonly x: number) {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also to clarify if it is just readonly without accessibility modifier, are we assuming it is public?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. This is the same as for properties. (class C { readonly x: number; })

@mhegazy
Copy link
Contributor

mhegazy commented May 11, 2016

Can you add a test for declaration emitter. i.e. // @declaration: true at the top of the file. see https://github.com/Microsoft/TypeScript/blob/master/tests/cases/compiler/declarationEmit_protectedMembers.ts for an example.

@mhegazy
Copy link
Contributor

mhegazy commented May 11, 2016

Also a test for:

  • readonly in method signature and method declaration
  • readonly in setter declarations
  • lambda function with a parameter named readonly.

@mhegazy
Copy link
Contributor

mhegazy commented May 11, 2016

Please add the word doc updates to the spec as well.

@mhegazy
Copy link
Contributor

mhegazy commented May 11, 2016

👍

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented May 12, 2016 •
edited
Loading

I think it'd be better to just file a spec bug and assign it to me or Anders.

@ghost ghost force-pushed the readonly_ctr branch from c32227d to 22ee90a Compare May 12, 2016 14:08
@ghost ghost mentioned this pull request May 12, 2016
Closed
@ghost ghost merged commit 7806de0 into master May 12, 2016
@ghost ghost deleted the readonly_ctr branch May 12, 2016 17:29
ghost pushed a commit that referenced this pull request May 12, 2016
Use NodeFlags.ParameterPropertyModifier rather than NodeFLags.Accessi…
b1cd3de 
…bilityModifier to detect parameter properties.

This is a continuation of #8555.
@ghost ghost mentioned this pull request May 12, 2016
Merged
@dsherret dsherret mentioned this pull request Aug 7, 2016
Closed
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@msftclas @yuit @mhegazy @DanielRosenwasser