lephyrus / ngx-translate-messageformat-compiler

Advanced pluralization (and more) for ngx-translate, using standard ICU syntax which is compiled with the help of messageformat.js.
MIT License
93 stars 30 forks source link

Do not use @Injectable decorator on TranslateMessageFormatCompiler (fixes #2) #3

Closed B4nan closed 7 years ago

B4nan commented 7 years ago

Here it is, hope its ok!

lephyrus commented 7 years ago

@B4nan I'm not using this with Angular 4 and AOT (yet), so I haven't seen this warning - thanks for reporting it and preparing a PR. The @Injectable annotation isn't needed, so removing it is a good idea anyway.

You've also added a messageformat import. I guess you did that because of this: https://github.com/B4nan/ngx-translate-messageformat-compiler/blob/d88fd171ba658c0f92a89ad5650ba143ae13b2d9/src/translate-message-format-compiler.ts#L8 However, messageFormat here is an instance of messageformat, but you are importing the constructor. Typescript knows the type without an import because I've added a simple messageformat.d.ts that is referenced in tsconfig.json.

While the naming may be confusing and the messageformat.d.ts file is not an ideal solution, your import is incorrect - could you remove it please?

B4nan commented 7 years ago

Well, originally i did not add the import, but typescript (2.5.2) was not able to compile that without the import...

ERROR in /path/to/project/node_modules/ngx-translate-messageformat-compiler/src/translate-message-format-compiler.ts (7,26): Cannot find name 'MessageFormat'.`
webpack: Failed to compile.

With the import added, it works ok, but its truth that my webstorm ide is complaining about the import being unused, so probably this is not how it should be done.

Not sure what is the best way to solve this, I am working with typescript just a few weeks now :]

I just found out that the import line also prints error when running tests. Will try to experiment with this a little bit more.

lephyrus commented 7 years ago

You get this error when running npm run build in the project directory? That's interesting, because I don't, with the same version of tsc. Are you sure it's picking up the right tsconfig.json (the one in the project root)? Just a thought.

One solution would be to just type the messageformat instance as any, and dropping messageformat.d.ts, which would mean losing some typing but certainly to an acceptable extent. Anyway, let's see what you find out. Anything dealing with this issue should be a separate commit or better yet, a separate PR. Thanks!

B4nan commented 7 years ago

Well the error is there when i call ng serve in my ng cli project. Running npm run build in this repo is actually broken with that import added.

Will remove it for now and update the PR so it will just remove the @Injectable decorator.

B4nan commented 7 years ago

Hmmm maybe I found a way to solve it - not sure why you have those typings separately in .d.ts file, but when I add:

interface MessageFormat {
  compile(value: any, lang: string): any;
}

instead of the import, it works ok.

Is it ok solution for you? Should I also remove the interface definition from .d.ts file so its not duplicated there? I think you don't really need it when you put the interface inside the service file like this.

lephyrus commented 7 years ago

I think I tried that but got errors because the constructor is then using a type that is not exported. Did you run npm run build again for this project? Anyway, let's just make this PR about @Injectable. Could you create another issue for the type error? I'll then try and deal with that later, or you can make another PR with the solution you propose.

B4nan commented 7 years ago

Looks like it has to stay there too, otherwise the npm run build breaks. I will leave this PR just about the AoT bug as you propose.

lephyrus commented 7 years ago

I think this has been solved - please re-open otherwise.