opral / monorepo

globalization ecosystem && change control SDK
https://opral.com
Apache License 2.0
1.15k stars 93 forks source link

decide on `onSetLanguageTag` throwing or not approach #1660

Closed samuelstroschein closed 10 months ago

samuelstroschein commented 10 months ago

Problem

Paraglide's strictness for "setting" the onSetLanguageTag callback is causing HMR issues.

samuelstroschein commented 10 months ago

Not changing the behavior atm. Throwing is OK because changing to not throwing is not a breaking change. Changing the behavior from not throwing to throwing would be a breaking change however.

samuelstroschein commented 10 months ago

Given that users run into this problem all the time, it is probably best to remove the strictness/throw. @LorisSigrist what do you think?

LorisSigrist commented 10 months ago

Yes I agree we need to be more permissive here. The question is how. Do we just allow many subscriptions to the language tag? What was the reasoning behind not doing that up until now?

samuelstroschein commented 10 months ago

Do we just allow many subscriptions to the language tag?

No subscriptions. Complexity for no value. Triggering multiple side-effects with different calls to onSetLanguageTag will a) lead to unexpected behavior b) memory leaks if the subscriptions are not disposed and c) everything can be handled in one call anyways.

I would simply not throw, let people re-assign the callback and warn in the docs that onSetLanguageTag will always be reassigned and cannot be set multiple times.

LorisSigrist commented 10 months ago

That makes sense. Should we also console.warn when a callback get's overridden?

samuelstroschein commented 10 months ago

Should we also console.warn when a callback get's overridden?

Could be super annoying in HMR.

We can also evaluate whether the bundler plugins resolve the HMR issue and not loosen the strictness of throwing. Having an explicit throw seems better than a warning hidden in the docs or console. Except ofc if HMR sets the callback but that can be avoided with bundler adapters.

If we expect everyone who uses bundlers to choose one of our adapters, we should be able to circumvent the issue of throws in HMR without loosening the strictness/immediate feedback "you can't set onSetLanguageTag multiple times".

@LorisSigrist can you explore in #1669 whether you can fix the HMR issues in those adapters? If you fix it in adapters, we need tests for it ;)

LorisSigrist commented 10 months ago

I'll see if I can make that happen!

LorisSigrist commented 10 months ago

@samuelstroschein We need to re-discuss this.

I have been able to reproduce the HMR issues thanks to @KraXen72. They only happen if the callback isn't inlined. In order for HMR to work, it needs to be able to reassign the callback. We can't make this work in the adapters like we thought.

I'll reopen #1699

samuelstroschein commented 10 months ago

@LorisSigrist that's what I thought. Okay, reassigning it is. I hope that we don't trigger regression bugs/unexpected behavior.