Skip to content

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

Merged
6 commits merged into
masterfrom
readonly_ctr
May 12, 2016
Merged

Fix #7590: Allow 'readonly' to be used in constructor parameters#8555
6 commits merged into
masterfrom
readonly_ctr

Conversation

@ghost

@ghost ghost commented May 11, 2016

Copy link
Copy Markdown

Fixes #7590

@msftclas

msftclas commented May 11, 2016

Copy link
Copy Markdown

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
Comment thread src/compiler/types.ts
Modifier = Export | Ambient | Public | Private | Protected | Static | Abstract | Default | Async,
Modifier = Export | Ambient | Public | Private | Protected | Static | Abstract | Default | Async | Readonly,
AccessibilityModifier = Public | Private | Protected,
// Accessibility modifiers and 'readonly' can be attached to a parameter in a constructor to make it a property.

@DanielRosenwasser DanielRosenwasser May 11, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

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
Comment thread src/compiler/types.ts
HasJsxSpreadAttribute = 1 << 30,

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

@yuit yuit May 11, 2016

Copy link
Copy Markdown
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

@mhegazy mhegazy May 11, 2016

Copy link
Copy Markdown
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..

@yuit yuit May 11, 2016

Copy link
Copy Markdown
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

yuit commented May 11, 2016

Copy link
Copy Markdown
Contributor

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

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

@yuit yuit May 11, 2016

Copy link
Copy Markdown
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?

@ghost ghost May 11, 2016

Copy link
Copy Markdown
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

mhegazy commented May 11, 2016

Copy link
Copy Markdown
Contributor

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

mhegazy commented May 11, 2016

Copy link
Copy Markdown
Contributor

Also a test for:

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

@mhegazy

mhegazy commented May 11, 2016

Copy link
Copy Markdown
Contributor

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

@mhegazy

mhegazy commented May 11, 2016

Copy link
Copy Markdown
Contributor

👍

@DanielRosenwasser

DanielRosenwasser commented May 12, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

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