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

Allow literal initializers of readonly properties in declaration files #26313

Merged
merged 4 commits into from
Sep 5, 2018

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Aug 8, 2018

And use them in declaration emit when required to preserve the widening behaviors of certain literal types.

Fixes #15881

Note: This also normalizes where the error span is placed for the ambient initializer error between variable declarations and properties - it is now always on the expression, rather than split between the expression and the = token.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

One refactoring comment to address before checking in.

Also, for an old PR like this, you should probably merge from master before merging to it. I forgot about this last week and broke the build.

return grammarErrorOnNode(node.initializer, Diagnostics.Initializers_are_not_allowed_in_ambient_contexts);
}
}
if (node.initializer && !(isConstOrReadonly && isStringOrNumberLiteralExpression(node.initializer))) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure you can move this inside the preceding if, as well as const isConstOrReadonly = ... above.

Then it would be worthwhile to invert the node.initializer check and return early.

@weswigham weswigham merged commit 62d8b85 into microsoft:master Sep 5, 2018
@weswigham weswigham deleted the fix-declaration-type-widening branch September 5, 2018 18:30
@@ -9,6 +9,6 @@ tests/cases/compiler/ambientStatement1.ts(4,20): error TS1039: Initializers are
!!! error TS1036: Statements are not allowed in ambient contexts.

export var v1 = () => false;
~
Copy link
Member

Choose a reason for hiding this comment

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

How is this readonly declaration?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not, the error span location was just normalized for the initializer forbidden error to always be on the initializer instead of sometimes on the initializer and sometimes on the equals token.

Copy link
Member

Choose a reason for hiding this comment

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

@weswigham makes sense. Sorry somehow I perceived as error removed. My bad.

@jacobdr
Copy link

jacobdr commented Nov 3, 2018

@weswigham Thanks for the feature. Any chance you can take a look at this issue of ours and offer some advice on what you think is the best way forward?

@carpben
Copy link

carpben commented Dec 30, 2018

Issue #15881 talks about type widening of readonly properties in classes. Will this PR solve the widening of readonly properties of object literals as well? For example:

interface IColors {
    readonly [k:string] : string
}
const COLORS : IColors = {
    red: "#f00"
}
interface IBasicReduxAction = {
    readonly type: string 
}
const someAction: IBasicReduxAction = {
    type:"SomeActionType"
}

After this release should the type of COLORS.red be recognized as "#f00" and the type of someAction.type be recognized as "SomeActionType"?

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.

5 participants