rbalet / ngx-translate-multi-http-loader

A loader for ngx-translate that loads translations with http calls
MIT License
77 stars 15 forks source link

use HttpBackend instead of HttpClient #18

Closed rbalet closed 2 years ago

rbalet commented 2 years ago

As commented here, if the HttpClient get intercepted, then it will cause an error & crash. Using the httpBackend safely remove this problem with no negative impact.

As soon as this pull request get accepted, I'll update the documentation

denniske commented 2 years ago

Hi, thanks for creating this PR. I am just wondering if somebody is using http interceptors and wants them to intercept the translation calls. What do you think?

denniske commented 2 years ago

I just realized that this would be a breaking change also because you will have to give the translation factory a http backend instead http client and this would confuse everybody who is upgrading. I think we should add a warning in the readme about interceptors instead with this code suggestion:

// Use the following code if you want your translation files to skip http interceptors
// See: https://github.com/ngx-translate/core/issues/1347

// AoT requires an exported function for factories
export function HttpLoaderFactory(httpBackend: HttpBackend) {
  const httpClient = new HttpClient(httpBackend);
  return new MultiTranslateHttpLoader(httpClient as any, [
    {prefix: "./assets/translate/core/", suffix: ".json"},
    {prefix: "./assets/translate/shared/", suffix: ".json"},
  ]);
}

@NgModule({
  declarations: [
    AppComponent
  ],
  imports: [
    BrowserModule,
    HttpClientModule,
    TranslateModule.forRoot({
      loader: {
        provide: TranslateLoader,
        useFactory: HttpLoaderFactory,
        deps: [HttpBackend],
      },
    }),
  ],
  providers: [],
  bootstrap: [AppComponent]
})
export class AppModule { }
rbalet commented 2 years ago

@denniske Maybe bumping a major release & mentioning it at the top with a big warning that it have to be change.

Also, if you wish I can add a new CHANGELOG.md file. People are use to read the changelog before downloading major release.

Also, if you let me I'll be glad to refactor a bit the code :)

What do you think ?

Another Idea I had was to make the suffix being .json by default with resource.suffix || .json

If this is ok for you, I'll close this merge request and do a big one :).

denniske commented 2 years ago

I like the idea with the default suffix.

The problem is I do not have much time for maintaining this repo and as such want to avoid breaking changes and the complications that arise with them. If you can take over maintaining the project I can give you access to the repo and the npm repo.

rbalet commented 2 years ago

@denniske I'll be glad to help you with this one, since actually I have to maintain a fork of it, which do not make a lot of sense. I'll be glad to have you review my changes, just to be sure you agree with me on what I'm doing.

Closing this merge request for the moment,

We'll do that work on Sunday ev.

rbalet commented 2 years ago

@denniske I went through the codebase & change it to the latest standard, I also enhanced it a little bit.

Changes

Question

Shall I completely remove the prefix, suffix logic ?
I have the feeling json being the standard, nobody never gonna use xml, and this just make the code & documentation being more complex for "nothing"

denniske commented 2 years ago

Shall I completely remove the prefix, suffix logic ?

Yes sounds good

rbalet commented 2 years ago

@denniske Done, please review the code, if it fits your standard

denniske commented 2 years ago

Looks good. I have merged the PR. Please delete your fork so I can transfer the repo to you.

rbalet commented 2 years ago

@denniske Nice.

delete your fork so I can transfer the repo to you. Done

Would you wish to push over npm a prerelease, that I can test it out if everything worked like expected ?

denniske commented 2 years ago

I have requested the repo transfer. You should get an email.

Yes, give me your npmjs username so I can invite you as maintainer for the package. Then you can do a prerelease.

rbalet commented 2 years ago

@denniske Saw that and accepted, thx.

This is my user name rbalet. And e-mail raphael.balet@outlook.com

denniske commented 2 years ago

Invite is on the way

denniske commented 2 years ago

Let me know if you got access, so I can remove myself as maintainer there.

rbalet commented 2 years ago

@denniske I got the access, everything seems ok. but you can stay maintainer, or wont you never have time for that anymore?

denniske commented 2 years ago

I haven't used this library myself since years so I will remove myself as maintainer. Thank you for taking over 👍

rbalet commented 2 years ago

@denniske Ok no problem, then thx an have a nice day