code-chunks / angular2-logger

A log4j inspired logger for angular 2.
MIT License
144 stars 41 forks source link

Type 'string' is not assignable to type 'Level' #39

Closed vcorr closed 8 years ago

vcorr commented 8 years ago

I get this error for at least the version 0.2.3. I can't yet update to 0.3.0.

Line: /angular2-logger/app/core/logger.ts:59:38

Related to this? https://basarat.gitbooks.io/typescript/content/docs/tips/stringEnums.html

changing the line 59 from: private _loadLevel = (): Level => localStorage.getItem( this._storeAs ); to private _loadLevel = (): Level => +localStorage.getItem( this._storeAs );

Seems to fix it (converts string to number)

langley-agm commented 8 years ago

Hello @vcorr ,

Could you share your Typescript compiler version and your tsconfig options?

vcorr commented 8 years ago

absolutely,

"typescript": "1.9.0-dev.20160503",

"compilerOptions": {

"target": "ES5",
"module": "commonjs",
"emitDecoratorMetadata": true,
"experimentalDecorators": true,
"moduleResolution" : "node",
"sourceMap": true

},

langley-agm commented 8 years ago

@vcorr I was able to reproduce your issue when I use typescript 1.9.0-dev.20160503, however that's not an official release yet.

I ran some tests and this issue is not happening with the latest official release: 1.8.10. I suggest you to use that instead.

However, I will leave this issue open in case it continues to occur after they officially release the next version, thank you !

k7sleeper commented 8 years ago

I run into the same issue with tsc v1.9.0-dev.20160601-1.0. I'm bound in using it because I need the path mapping features in conjunction with JSPM :-(

Is it an error of the TypeScript compiler?

langley-agm commented 8 years ago

@k7sleeper To clarify:

It's an error thrown by the Typescript compiler. Not precisely an error of the compiler. The error is in the logger's code if the following official version of Typescript ships the way it is now ( in 1.9.0-dev ). If it does, it will get fixed appropriately.

k7sleeper commented 8 years ago

OK, I understand.Tx.

I wonder why the compiler reads ./app/core/logger.ts although ./app/core/logger.d.ts is present and sufficient for defining the API of a 3rd party library.

k7sleeper commented 8 years ago

If I add a "typings": "core.d.ts",to package.json and remove app/core/*.ts then there's no compiler error.

langley-agm commented 8 years ago

@k7sleeper core.d.ts gets generated by the compiler out of core.ts if the .d.ts is present is only because it has already been compiled.

k7sleeper commented 8 years ago

That's clear!

Setting the typings attribute in package.json to core.d.ts let's the TypeScript compiler use core.d.ts when compiling import {Logger} from "angular2-logger";. That's what's needed to avoid compiling whole angular2-logger sources. Angular2 folks do the same. They set the typings attribute in package.json.

langley-agm commented 8 years ago

@k7sleeper sorry, miss-clicked the close button.

let's the TypeScript compiler use core.d.ts

The typescript compiler doesn't use .d.ts files, it generates them. If you wanted to compile .d.ts files you'd have to write them yourself, or compile once to generate the .d.ts files and a second config that compiles those into js. However, .d.ts files are merely the definitions of the API, you'd be compiling empty definitions with no implementation whatsoever, you'd be able to call the functions but they wouldn't be doing anything.

Also package.json doesn't tell the compiler how to compile, that's tsconfig.json.

Just clarifying some things to avoid miss-conceptions. I'll be carefully reviewing your pull requests with more calm as soon as I get a chance.

Thanks for contributing !

k7sleeper commented 8 years ago

The typescript compiler doesn't use .d.ts files, it generates them.

No, it also uses them for getting type information when processing import statements if no .ts file exists which is normally the case if you import a 3rd party library (i.e. angular2-logger).

Also package.json doesn't tell the compiler how to compile, that's tsconfig.json.

No!

The typings attribute of package.json tells the compiler which type definition file (.d.ts) should be used if the client code imports the package (i.e. import {Logger} from "angular2-logger").

The main attribute of package.json tells the loader (i.e. SystemJS) which JavaScript file should be loaded if the client code imports the package (i.e. import {Logger} from "angular2-logger").

langley-agm commented 8 years ago

@k7sleeper

which is normally the case if you import a 3rd party library

RxJS does include .ts files and so did Angular until apparently some releases ago. What other libraries are you basing on to make this statement ?

if no .ts file exists

But they do exist, so why do you want to add this?

k7sleeper commented 8 years ago

@langley-agm

This issue is caused because the Typescript compiler prefers the .ts files to .d.ts files.

So, if you dont deliver the .tsfiles this issues can be closed. This issues can be also closed if you deliver the .ts files at another place, maybe in a subfolder src.

k7sleeper commented 8 years ago

Generally, I think, delivering the .ts files in a 3rd party package like this is ok for dokumentation. But what's really needed are the JavaScript files for runtime and the .d.tsfiles for TypeScript compile-time, or in other words: the executable code and the TypeScript API definition.

If the non-executable code is delivered, IMO it should be located at a place in the package where the Typescript compiler does not look at by default.

langley-agm commented 8 years ago

@k7sleeper

This issue is caused because the Typescript compiler prefers the .ts files to .d.ts files.

Actually this issue is caused by a change in Typescript syntaxis in a release that is not yet final. Removing or moving .ts is a workaround, not a fix, the underlying issue will still be there.

So, if you dont deliver the .ts files this issues can be closed.

Since I've started using Angular 2 I've had to dive into the source code (.ts files ) so many times, I really see the value in delivering these files, I don't want to go to GitHub and look for the file manually every time I want to know how something works, its easier to just have the IDE take me to the file automatically because I already have it in my environment.

If the non-executable code is delivered, IMO it should be located at a place in the package where the Typescript compiler does not look at by default.

The fact that typescript is looking at .ts files of ignored libraries that it shouldn't compile when the .d.ts files exist it's an issue in Typescript side IMHO, I suggest you to open a ticket in their repo and add a link to this issue.

This issue can be also closed if you deliver the .ts files at another place, maybe in a subfolder src.

This is still a workaround but I'm not totally against this idea, I'll consider it when Typescript actually releases the version in which this issue is happening, who knows, maybe they'll fix it in some other way by then.

philjones88 commented 8 years ago

getting this too on the beta release of Typescript 2.0 that was released last night.

langley-agm commented 8 years ago

Hi @philjones88 Thanks for the uptate !

philjones88 commented 8 years ago

Is this ever going to be fixed? Are you accepting pull requests for that 1 line change?

langley-agm commented 8 years ago

@philjones88 As I mentioned earlier, this issue is not happening in the latest official release version of Typescript (1.8), If you don't want to see this error I recommend using a version already released ( not alpha, nor beta ).

I do accept pull requests, however I think the root cause of the issue is not this one line of code but the fact that typescript even tries to compile it when you already have it as a compiled dist. What if you wanted to keep your project using an old version of Typescript?

I think @k7sleeper was on the right track trying to find a way for his project to avoid compiling this TP Library, however a way to do this without removing the source files from the package hasn't been found yet. If you find something like this and you make a pull request I'd be more than happy to take a look at it.

paralin commented 8 years ago

Using 2.0 this is still broken, I don't think saying "don't use an alpha release" is really a solution

langley-agm commented 8 years ago

@paralin I never said it was a solution my friend. I said 2.0 is not an officially released version and if you used one that was official you won't encounter this issue. By using alpha and beta releases you are accepting the fact that you will encounter this type of unsolved issues.

The root cause of this issue is the fact that a project tries to compile a third party library when it shouldn't and personally I haven't found the solution for this if you are interested to pitch in with PRs or pointers of good information you are more than welcome to do so =)

paralin commented 8 years ago

I solved it by just putting a + in front of that one statement as someone else said before. I have no problem with my app compiling the module. Maybe just release that as a stopgap fix so I don't have to use a git reference in my packages list.

langley-agm commented 8 years ago

In my must humble opinion that's a workaround and doesn't fix the root cause, the fact that you don't have a problem with it doesn't mean it's correct. If by the time 2.0 is officially released there's no other solution, I'll apply that fix, until then I don't want to hide the underlying issue.

I apologize for the inconveniences this causes. But like I mentioned, by using unnoficial releases you are accepting to deal with this kind of issues, which is only an error in the console that doesn't affect the lib's functionality at all.

paralin commented 8 years ago

Regardless I don't see the problem with fixing your code regardless of if it's being interpreted at the time you compile the lib or I do. They are two separate issues. Just fix the one minor problem and leave the other issue to be fixed.

langley-agm commented 8 years ago

The code is written using Typescript 1.8, and it works fine, there's nothing to fix. A further version of the library might be written in a newest Typescript version, might not. If it does, I'll address this issue, that's why I keep it open.

When you create a library using Java 7 you don't have to recompile it in order to use it with Java 8 projects do you? You don't suddenly need to convert all your classes into using lambdas instead just because a new version was released that supports them do you?

The actual compiled lib code is JS and you can use it just fine. The source is there for reference not so you compile it.

This is an issue in the Typescript side, not in this lib. You can raise a defect in the Typescript project if you wish to do so.

paralin commented 8 years ago

"When you create a library using Java 7 you don't have to recompile it in order to use it with Java 8 projects do you? You don't suddenly need to convert all your classes into using lambdas instead just because a new version was released that supports them do you?"

I think you'll find that the moment more people than just yourself use a library, supporting other versions than the one you use will become something you'll want to do. Case and point. I'm not going to revert the entire Typescript version of my project just because you refuse to add a single character change to your repo.

This is an issue in the Typescript side, not in this lib. You can raise a defect in the Typescript project if you wish to do so.

This is NOT a Typescript bug. You're implicitly casting a string to a integer here. This is invalid, and if it wasn't warned about in a previous version of Typescript that's the bug.

The fix is to cast the string value to an integer with a single character - the + symbol, which by the way is valid in every version of Typescript.

The sensible choice here is to just make the single character change. That's all you have to do to make this work with the newer versions of Typescript. It's really that simple. Just do it and save us all the trouble of using third party forks...

langley-agm commented 8 years ago

When you create a library using Java 7 you don't have to recompile it in order to use it with Java 8 projects do you? You don't suddenly need to convert all your classes into using lambdas instead just because a new version was released that supports them do you?

You are completely missing the point. What I'm trying to say is that you can use a library build with Java 7 in a Java 8 project because the language supports it ! Your library doesn't have to support it, the library gets distributed as a jar file which you no longer have to compile cause its already compiled. Same with .ddls there's no reason to recompile a library that's already distributed as a compiled artifact.

The fact that typescript makes you compile third party libraries IS a Typescript issue.

This is NOT a Typescript bug. You're implicitly casting a string to a integer here. This is invalid, and if it wasn't warned about in a previous version of Typescript that's the bug.

This is not invalid in Javascript nor in Typescript 1.8, its actually one of the biggest features/advantages of it. Its invalid in an unreleased version of Typescript with a new feature which I don't currently support because its not even released yet. The fact that Typescript doesn't support old versions of his own code its a lack in their side.

paralin commented 8 years ago

I think you're missing the point. I'm saying if you make 1 commit that adds a single character + you will fix all of these errors for us, your users. Fix the harder problems later.

langley-agm commented 8 years ago

Or... if you make a defect ticket in Typescript you might get this and many other future issues dissapear.

I do know what you are saying, doesn't mean I have to agree with you.

I already stated my point, I already heard yours and I really apologize for the troubles this is causing but you shouldn't be using alpha/beta releases in production environments, and if you are using them in personal/small projects, a warning in the console which doesn't affect functionality shouldn't matter. That's my stance and I agree to disagree with you.

IMHO Software Quality is getting more affected by the day by things like using unreleased software in production projects, redoing instead of fixing, patching instead of finding the root cause. And I see this everyday in the daily tools I use. The common usage of unreleased software in production is affecting both companies and software developers alike. I might be wrong but this is what I've noticed and it's what I think, I do not want to encourage this.

paralin commented 8 years ago

Typescript 2.0 is already released, 2.1 is the unreleased version right now, a lot of stuff is now using 2.0 as it IS compatible (for the most part) and has a lot of improvements.

You're maintaining an extremely minor piece of software here man. You don't have the position to influence the entire industry as a whole as you seem to believe. Quit wasting your time on this crap, fix your bug (literally 1 character change), or refuse to do so, but don't try to lecture everyone on best practice. I'm not going to argue with you any more here - it's clear you'd rather argue best practice than fix your buggy code. To each his own, I guess. Best of luck with the project.

langley-agm commented 8 years ago

@paralin The latest official release is 1.8.10 not 2.0. You can see it in the official webpage, you can see it in npm and you can see it when you install it without specifying a version.

image

image

You don't have the position to influence the entire industry as a whole as you seem to believe.

No, I do not believe that, I just try to do my minor part.

or refuse to do so, but don't try to lecture everyone on best practice.

I already refused several times =). I am not lecturing anyone I am trying to explain my reasoning for it. If you don't agree with it that's totally fine.

Best of luck with the project.

Thank you !

aperger commented 8 years ago

Hi,

I am using Atom IDE with atom-typescript package [offered by MS]. The installed Typescript is: ~$ tsc --version Version 1.8.10

A) If I build my project under Atom IDE with F6 hotkey (I included your package of course, v0.3.0), the error message is raising. B) If I build my project in command line: tsc No error is raising.

Why???

Maybe your tsconfig.json is different than mine or Atom use different settings for compilation. BUT. The implicit conversions are valid among integer and enum, not between strings and enum. As I know.

langley-agm commented 8 years ago

Hi @aperger

I've noticed this in IDEs like IntelliJ where the IDE ignores your configuration and uses its own compiler:

image

Probably Atom has a configuration like this where you can select the typescript version you want to use.

Also remember that npm can install a typescript version locally and a different one globally, make sure the IDE is using the one you want or install the same version locally and globally.

Those are the two things I can think of about why this could be happening to you. Let me know if one of them fixes it for you.

The implicit conversions are valid among integer and enum, not between strings and enum.

That link say its ok implicit conversion between integer and enum I agree, however it never says its not ok between strings and enum, that part its your own words. This is an expected behaviour in Javascript and in Typescript 1.8 and before:

You can try this in your browser's console:

var a = "1"; console.log(++a);

You'll see 2 gets printed.

Newer unreleased versions of Typescript don't allow a String to be assigned to an enum anymore.

Some people may prefer this, some people might not but that's the way it was designed and I want to respect that.

If a new version of this lib is written in a newer version of Typescript, I'll make the correction for that version. As of now this library does not support a beta of Typescript.

aperger commented 8 years ago

Thanks for the detailed explanation....

Additional note: I have tried your code. var a = "1"; console.log(++a); As I expected your code is working in the browsers (executed as JavaScript code), as you expected, too. But I could not compile your code with typescript compiler (tsc v.: 1.8.10, I used command line). error TS2356: An arithmetic operand must be of type 'any', 'number' or an enum type.

paralin commented 8 years ago

It doesn't support a beta of Typescript despite requiring only a 1 character change to support the beta....

k7sleeper commented 8 years ago

Only to recall: this problem is not caused because of this "one character", it's caused because TypeScript compiler compiles the .ts files instead of using the .d.ts files when resolving import "angular2-logger";. For me, as a user of the library, if i add import "angular2-logger"; to one of my source files I don't care a pap for it that angular2-logger is build by TypeScript compiler (1.8, 1.7. or even 1.0), or Babel or whatever you want (CoffeeScript, Closure, Kotlin, ...). If I add import "angular2-logger"; to one of my source files I expect some *.js files somewhere on my disk and ideally one *.d.ts nearby the *.js files. Nothing more (ok, a package.json with a correct main and typings attribute, too).

Therefore, other libraries separate the source files (*.ts) from the generated files (*.d.ts, *.js) in the distribution. But this library does not ...

Overall, it's good style to separate source files from generated files, and as a side effect it would solve this problem.

langley-agm commented 8 years ago

as a user of the library, if i add import "angular2-logger"; to one of my source files I don't care a pap for it that angular2-logger is build by TypeScript compiler (1.8, 1.7. or even 1.0)

EXACTLY !

This is the approach I will try first ( separating the definition files from the source ) when 2.0 gets released. Sooner If I get some free time from work. PRs are welcome as well.

langley-agm commented 8 years ago

Looks like this is an already known issue is Typescript:

https://github.com/Microsoft/TypeScript/issues/2568

Sadly the apparent fix is only half implemented and still needs:

https://github.com/Microsoft/TypeScript/issues/4433

I have commited a temporary manual fix and added a new checkbox in the welcome README in order to track a proper fix later on once Typescript has a way to do it automatically.