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

feat(plugins/i18next): Use path to decide default namespace #3025

Closed
wants to merge 3 commits into from

Conversation

MrTwixxy
Copy link

Disclaimer: I haven't tested it yet, since Im unable to build myself

This fixes the default behavior of using the first namespace when there's none defined.
Useful when you have a monorepo and only want 1 sherlock project

Copy link

changeset-bot bot commented Jul 24, 2024

⚠️ No Changeset found

Latest commit: 7ec9773

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

github-actions bot commented Jul 24, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@felixhaeberle
Copy link
Contributor

felixhaeberle commented Jul 24, 2024

@MrTwixxy can you sign the CLA?

  1. do you have a link where on the i18next page this behavior is described as we need to make sure not to break existing setups?
  2. have you added a test to the test file which should mimic this behavior? this is needed to make sure everything is working smoothly with further updates.

thank you for the PR:)

@MrTwixxy
Copy link
Author

I have read the CLA Document and I hereby sign the CLA

github-actions bot added a commit that referenced this pull request Jul 24, 2024
@felixhaeberle
Copy link
Contributor

@MrTwixxy I've approved the pipeline so we can check if the changes leads to errors.

@MrTwixxy
Copy link
Author

I've fixed the tests and added a new one.
The behavior isn't 'defined' somewhere, but the default behavior isn't either.
I don't expect things to break for the following reasons:

  • Nothing changed if namespaces are used how they normally are
  • If you aren't using them how they normally are, it's just using always using the default, which still happens if the path doesnt match any namespace

@felixhaeberle
Copy link
Contributor

hey, we have to discuss this change internally as this would mean that every plugin now needs some kind of strategy on how to do documentPath's, see https://github.com/MrTwixxy/inlang-monorepo-fork/blob/7ec9773714d9875bab0b3778a7567b4b103f1aed/inlang/source-code/versioned-interfaces/plugin/src/customApis/app.inlang.ideExtension.ts#L43

@MrTwixxy
Copy link
Author

Having the argument passed into the function doesn't necessarily mean that it has to be used right?

@samuelstroschein
Copy link
Member

@felixhaeberle i tend to agree with @MrTwixxy. just because the argument exists, doesn't mean that a plugin needs to use documentPath.

@samuelstroschein
Copy link
Member

i propose to close this pr given that we will re-implement the custom API opral/inlang-sdk#125

@github-actions github-actions bot locked and limited conversation to collaborators Aug 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants