Skip to content

Conversation

@rbalet
Copy link
Owner

@rbalet rbalet commented Oct 21, 2022

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

As commented [here](ngx-translate/core#1347), 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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Owner Author

rbalet commented Oct 21, 2022

@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
Copy link
Collaborator

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
Copy link
Owner Author

rbalet commented Oct 21, 2022

@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
Copy link
Owner Author

rbalet commented Oct 22, 2022

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

Changes

  • Accept string[] | ITranslationResource[], since the library do not accept something else than .json file. (And for what I see it will never do), then I made it accept a list of string[]` to be faster & nicer for developer to use.
  • Make default suffix value being .json
  • package.json: Avoid having useless forced dependencies and move them into peerDependencies
  • package.json: Correct the version declaration
  • Bump the version to v9.0.0
  • Rewrite the entire README
  • README.md : use i18n instead of translations to follow more the ngx-translate way of doing
  • Explain both string[] use and ITranslationResource[] uses
  • Add breaking changes
  • Change note.txt to be CONTRIBUTING.md

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
Copy link
Collaborator

Shall I completely remove the prefix, suffix logic ?

Yes sounds good

@rbalet
Copy link
Owner Author

rbalet commented Oct 22, 2022

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

@denniske denniske merged commit 4923daa into rbalet:master Oct 22, 2022
@denniske
Copy link
Collaborator

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

@rbalet
Copy link
Owner Author

rbalet commented Oct 22, 2022

@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
Copy link
Collaborator

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
Copy link
Owner Author

rbalet commented Oct 22, 2022

@denniske Saw that and accepted, thx.

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

@denniske
Copy link
Collaborator

Invite is on the way

@denniske
Copy link
Collaborator

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

@rbalet
Copy link
Owner Author

rbalet commented Oct 22, 2022

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

@denniske
Copy link
Collaborator

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

@rbalet
Copy link
Owner Author

rbalet commented Oct 22, 2022

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

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.

2 participants