Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🩹 (fiberi18n): Fix request concurrency errors #723

Merged
merged 18 commits into from
Sep 5, 2023

Conversation

Skyenought
Copy link
Member

fix #721

  1. Add more Signature in docs
  2. Change MustGetMessage(param interface{}) (string, error), GetMessage -> MustLocalize, Localize , Similar to github.com/nicksnyder/go-i18n/v2
  3. use ctx as param, avoid concurrency errors

@ReneWerner87 ReneWerner87 added ☢️ Bug Something isn't working ❗ BreakingChange labels Aug 28, 2023
@ReneWerner87
Copy link
Member

ReneWerner87 commented Aug 28, 2023

pls update the go.mod version to /v2

like this
b758faf

@gaby
Copy link
Member

gaby commented Aug 28, 2023

@Skyenought Can you add a test for localize(). It should be good to go after that

fiberi18n/i18n_test.go Outdated Show resolved Hide resolved
fiberi18n/go.mod Show resolved Hide resolved
@gaby
Copy link
Member

gaby commented Aug 29, 2023

@Skyenought you may need to run go mod tidy since the go version change to 1.19

@gaby
Copy link
Member

gaby commented Aug 29, 2023

May as well also add 1.21 to the test file

@Skyenought
Copy link
Member Author

Skyenought commented Aug 29, 2023

@gaby After I run go mod tidy, go.sum and go.mod don't change.

So I don't know what went wrong.

@gaby
Copy link
Member

gaby commented Aug 29, 2023

@gaby After I run go mod tidy, go.sum and go.mod don't change.

So I don't know what went wrong.

Try this, delete your go.sum file then run go mod tidy

@Skyenought
Copy link
Member Author

@gaby After I run go mod tidy, go.sum and go.mod don't change.
So I don't know what went wrong.

Try this, delete your go.sum file then run go mod tidy

Useless, go.sum is unchanged from its original content.

Copy link
Member

@gaby gaby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove 1.18 from test

@Skyenought
Copy link
Member Author

It doesn't look like it's working.

@gaby
Copy link
Member

gaby commented Aug 29, 2023

I'm not sure what's going on, the CI for main is also failing.

Lets try this, make your go.mod the following the run go mod tidy

module github.com/gofiber/contrib/fiberi18n/v2

go 1.19

require (
	github.com/gofiber/fiber/v2 v2.49.0
	github.com/nicksnyder/go-i18n/v2 v2.2.1
)

@Skyenought
Copy link
Member Author

Why paseto's CI was wrong? It hasn't changed at all.
QQ截图20230829113934

@gaby
Copy link
Member

gaby commented Aug 29, 2023

Why paseto's CI was wrong? It hasn't changed at all. QQ截图20230829113934

Yeah, but the unit tests are failing too

@Skyenought Skyenought force-pushed the i18n/bug branch 4 times, most recently from 6e28e91 to 77c14f1 Compare August 29, 2023 12:04
@gaby
Copy link
Member

gaby commented Aug 29, 2023

==================
WARNING: DATA RACE
Write at 0x000001147540 by goroutine 17:
  github.com/gofiber/contrib/fiberi18n/v2.(*Config).initLocalizerMap()
      /home/runner/work/contrib/contrib/fiberi18n/i18n.go:64 +0x292
  github.com/gofiber/contrib/fiberi18n/v2.New()
      /home/runner/work/contrib/contrib/fiberi18n/i18n.go:22 +0x1ee
  github.com/gofiber/contrib/fiberi18n/v2.TestLocalize()
      

@Skyenought
Copy link
Member Author

Finally solved the go mod tidy problem

@gaby
Copy link
Member

gaby commented Aug 29, 2023

Do we really need all thos go mod tidy? That can cause inconsistent tests

@Skyenought
Copy link
Member Author

i don't know, it just test.Maybe a PR is needed to modify the github action

@Skyenought Skyenought force-pushed the i18n/bug branch 2 times, most recently from d4f0dd5 to 0db09d7 Compare September 3, 2023 03:04
@gaby
Copy link
Member

gaby commented Sep 4, 2023

@Skyenought Can you update your branch with master. The go.mod changes were done there to avoid release issues

@ReneWerner87
Copy link
Member

i did the update

fiberi18n/i18n.go Outdated Show resolved Hide resolved
fiberi18n/i18n.go Show resolved Hide resolved
fiberi18n/i18n.go Outdated Show resolved Hide resolved
fiberi18n/README.md Outdated Show resolved Hide resolved
@ReneWerner87 ReneWerner87 merged commit 64e52f3 into gofiber:main Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
❗ BreakingChange ☢️ Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 [Bug]: i18n middleware panics
4 participants