microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
101.34k stars 12.54k forks source link

External variable is considered a constant or read-only wrongly #6751

Closed plantain-00 closed 8 years ago

plantain-00 commented 8 years ago
error message when run tsc

error TS2450: Left-hand side of assignment expression cannot be a constant or a read-only property.

a.ts
export let value = 1;
b.ts
import * as a from "./a";

function load() {
    a.value = 2;
}
tsconfig.json
{
    "compilerOptions": {
        "target": "es5",
        "module": "commonjs"
    }
}
environment
>tsc -v
Version 1.9.0-dev.20160129
Arnavion commented 8 years ago

That is how ES6 modules work - imports are constant. Only the module that exports them can change them, which then propagates to all modules that import them.

a.ts

export let value = 1;

export function setValue(newValue: number) {
    value = newValue;
}

b.ts

import * as a from "./a";

function load() {
    a.setValue(2);
}
plantain-00 commented 8 years ago

@Arnavion I'm afraid not that. Constant works like this:

const b = {
    c: 1
};
b = null; // I can not do this
b.c = 2; // I can do this

The generated js I expects is: a.js:

"use strict";
exports.value = 1;

b.js:

"use strict";
var a = require("./a");
function load() {
    a.value = 2;
}

If I invoke load() in node.js(I did this earlier), it works, the value of a's value changes.

BTW, the code works with typescript@1.7 and typescript@1.8.0-dev.20160117, but not work with typescript@1.9.0-dev.20160129, I upgraded typescript today, then I find the error.

Arnavion commented 8 years ago

Yes, if you compile ES6 to ES5 require then you get the behavior you expect.

That does not mean ES6 modules allow it. As I said the import binding is immutable and is not allowed to be modified by ES6 spec.

The reason it worked before and errors now is that it was a long-standing bug https://github.com/Microsoft/TypeScript/issues/2456 that was only fixed a few days ago by https://github.com/Microsoft/TypeScript/pull/6532

plantain-00 commented 8 years ago

Alright, I will use require instead.

ielcoro commented 8 years ago

I still don't get why it enforces es6 module format when using "module": "amd" in tsconfig.json. I have a perfectly working durandal application that I can't migrate to TypeScript 2.0 because of this, as durandal declaration files, as many others, use modules with exported vars:

declare module 'durandal/binder' {
    interface BindingInstruction {
        applyBindings: boolean;
    }

    /**
     * Called before every binding operation. Does nothing by default.
     * @param {object} data The data that is about to be bound.
     * @param {DOMElement} view The view that is about to be bound.
     * @param {object} instruction The object that carries the binding instructions.
    */
    export var binding: (data: any, view: HTMLElement, instruction: BindingInstruction) => void;

    /**
     * Called after every binding operation. Does nothing by default.
     * @param {object} data The data that has just been bound.
     * @param {DOMElement} view The view that has just been bound.
     * @param {object} instruction The object that carries the binding instructions.
    */
    export var bindingComplete: (data: any, view: HTMLElement, instruction: BindingInstruction) => void;

When using those declaration files, all the imports, that previously worked fine, now import readonly constants, so this will not compile:

import * as binder from 'durandal/binder';

`binder.binding = (obj, view) => {
                        //do something
                    };`

Despite, as specified in tsconfig.json, all the imports being transpiled to amd format, so in the final JavaScript the imports end up being writable:

define(["require", "exports",  'durandal/binder'], function (require, exports, binder,) { ... }

Shouldn't be allowed to have writable imports when using amd or other module formats different from the ts specification?

Also, note that in this example changing all the imports (despite the hard work) to require does not work either, as you can have a typed variable from of a module called 'durandal/binder':

var binder : "durandal/binder" = require('durandal/binder')
//binder ends up being literal type 

var binder : durandal/binder = require('durandal/binder')
//this expression does not compile

var binder = require('durandal/binder');
//binder ends up being of type any`
RyanCavanaugh commented 8 years ago

You can write

import binder = require('durandal/binder')

The suggestion that we ignore ES6 semantics when you use ES6 syntax seems to miss the point of downleveling. If you want AMD/CommonJS behavior, use the CommonJS-oriented syntax (import / require). Allowing you to do things using ES6 syntax which are specifically forbidden in ES6 is just setting you up for future failure when ES6 module implementations are widespread and you want to transition off of what is essentially a polyfill.

ielcoro commented 8 years ago

Thanks @RyanCavanaugh

The import = require worked.

I can see the conflict with downleveling and changing semantics, however, I think the current state gives more headaches than what it solves. Using ES6 syntax in most of the places give us more power, like importing specific types from a module, that would need importing the whole module with AMD/CommonJS syntax, and the current downleveling works out the details (importing the whole module and referencing it accordingly). Also, migrating an application that uses import = require to ES6 modules, will be a pain, finding and changing all those imports for ES6 syntax.

Migrating to ES6 modules will be a pain anyway, as libraries must be changed to take into account the mutability of the exported variables from a module, so it could be made easier if there was a unified syntax to importing from typescipt, that adapts to the scenario in hand, reducing the weak Find and Replace when the migrating to ES6 modules (if that happens sometime in the near future ;-))

Bobris commented 8 years ago

This is probably most significant breaking change when migrating from 1.8 to 2.0. Especially in tests when it is used for mocking.

cervengoc commented 8 years ago

Allowing you to do things using ES6 syntax which are specifically forbidden in ES6 is just setting you up for future failure when ES6 module implementations are widespread and you want to transition off of what is essentially a polyfill.

@RyanCavanaugh But if I understood correctly this error should only be suppressed if there is "target": "es5" in tsconfig.json. Whenever we might target native ES6 in the future, we will get the error and be forced to correct it. I think suppressing this error when targeting ES5 or lower is reasonable.

RyanCavanaugh commented 8 years ago

I don't understand what's desirable about making the abstraction leaky on purpose. The entire point of downleveling is to make it appear as if ES6 modules were widely available, not to give you new syntax for ES5 behavior.

If you want ES5 semantics, there's an easy way to do that: Use ES5 syntax. If you use ES6 syntax, you're going to get ES6 semantics. That's not negotiable.

cervengoc commented 8 years ago

It's a matter of viewpoint, I see this syntax as TypeScript syntax, not ES6 syntax. And as so, it should probably compile to ES5 without this error message. because I think the fact wether I want ES6 semantics or not is more related to the specified target. But if one declares it as ES6 syntax, then you are completely right. But then I'd probably disallow using this syntax when targeting ES5.

Anyway, this is just a respectful opinion, using require syntax is completely OK I think, just wanted to share my thoughts.

netpedro-com commented 7 years ago

Answering the initial question of @plantain-00, to supress error TS2450 message, just type (<any>a).value = 2; More info: http://stackoverflow.com/a/38833920/1882644