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

MESDK-77 add @inlang/sdk/v2 with MessageBundle and related types #2700

Merged
merged 10 commits into from
May 9, 2024

Conversation

jldec
Copy link
Contributor

@jldec jldec commented May 3, 2024

Introduces v2 MessageBundle and related types, extracted from #2655
Resolves opral/inlang-sdk#40

  • Export types from @inlang/sdk/v2
  • Remove local declarations
  • Add draft WIP v2 crud interfaces to InlangProject
    • e.g. project.messageBundles.get() and project.messages.getAll()
  • Inline LanguageTag from versioned-interfaces

Usage

import type { Message, MessageBundle } from '@inlang/sdk/v2'

const myMessageBundle: MessageBundle = {
  // to be filled in
}

In case of type name conflicts with existing Message type

import type * as V2 from '@inlang/sdk/v2'

const myMessageBundle: V2.MessageBundle = {
  // to be filled in
}

A message bundle is a collection of Messages which are based on a subset of ICU MessageFormat 2.

A bundle has an id (and optional aliases) which can be used to reference messages from UI code. Each message in the bundle targets a different locale.

export type Message = Static<typeof Message>
export const Message = Type.Object({
	locale: Locale,
	declarations: Type.Array(Declaration),
	selectors: Type.Array(Expression),
	variants: Type.Array(Variant),
})

export type MessageBundle = Static<typeof MessageBundle>
export const MessageBundle = Type.Object({
	id: Type.String(),
	alias: Type.Record(Type.String(), Type.String()),
	messages: Type.Array(Message),
})

Copy link

changeset-bot bot commented May 3, 2024

🦋 Changeset detected

Latest commit: b9eccb7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 29 packages
Name Type
@inlang/sdk Patch
@inlang/badge Patch
@inlang/cli Patch
@inlang/doc-layout-component Patch
@inlang/editor Patch
@inlang/github-lint-action Patch
vs-code-extension Patch
@inlang/message-bundle-component Patch
@inlang/rpc Patch
@inlang/settings-component Patch
@inlang/telemetry Patch
@inlang/cross-sell-ninja Patch
@inlang/plugin-i18next Patch
@inlang/plugin-json Patch
@inlang/plugin-m-function-matcher Patch
@inlang/plugin-next-intl Patch
@inlang/plugin-t-function-matcher Patch
@inlang/paraglide-unplugin Patch
@inlang/paraglide-js-e2e Patch
@inlang/sdk-load-test Patch
@inlang/server Patch
next-js-testapp Patch
@inlang/sdk-multi-project-test Patch
@inlang/paraglide-rollup Patch
@inlang/paraglide-vite Patch
@inlang/paraglide-webpack Patch
@inlang/paraglide-astro Patch
@inlang/paraglide-sveltekit Patch
@inlang/paraglide-sveltekit-example Patch

Not sure what this means? Click here to learn what changesets are.

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

@jldec jldec requested a review from NilsJacobsen May 3, 2024 16:31
Copy link
Member

@samuelstroschein samuelstroschein left a comment

Choose a reason for hiding this comment

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

Wondering if releasing a v2 of @inlang/message isn't better. Apps using the SDK now have name collisions:

import { Message } from "@inlang/sdk" 

What message is imported here?

Recommendation:

  • Release V2 prerelease of @inlang/message instead of having a new package
  • Suffix all types with V2 e.g. MessageV2, PatternV2, etc. to avoid name collisions

(We will run into this problem again with a potential v3. Seems easier to then again suffix MessageV3 instead of having a new package and 3 potential name collisions)

Comment on lines 89 to 94
export type LocalDeclaration = Static<typeof InputDeclaration>
export const LocalDeclaration = Type.Object({
type: Type.Literal("local"),
name: Type.String(),
value: Expression,
})
Copy link
Member

Choose a reason for hiding this comment

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

Do we need local declarations for v1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we want to be able to import MF2 using this, right?

Copy link
Member

Choose a reason for hiding this comment

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

Why do we want to import MF2 for the MVP?

  • nobody uses MF2
  • let's ship only what we need to unblock apps namely (variants)
  • ship in a way to make the data structure future proof

MF2 is still changing. anything we don't need i wouldn't introduce now

Copy link
Member

Choose a reason for hiding this comment

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

Here still seems to be some unalignment. Let's achieve consensus @jldec @LorisSigrist @martin-lysk.

I wouldn't risk adding features from MF2 that apps don't need right now to support variants (pluralization, gendering) as they still change. I don't know why the need to import MF2 is recurringly coming up. We don't need to import MF2 just yet. Nobody uses MF2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought we needed to support formatting and markup features in inlang messages, and that local declarations are required to support patterns that embed modified (non-local) inputs. The local declaration defines how the input is modified instead of nesting functions inline in the pattern.

If we can do everything we need without locals, I'm happy to remove it.

@martin-lysk @LorisSigrist please let me know if you think this makes sense (or not) given what paraglide users have requested, or based on what you think you need for the paraglide compiler.

Copy link
Contributor Author

@jldec jldec May 7, 2024

Choose a reason for hiding this comment

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

Editing nested functions without locals also feels harder from UI/UX perspective, than doing it with locals. Do you agree @NilsJacobsen ?

Copy link
Member

Choose a reason for hiding this comment

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

I would go step by step to enable pluralization first. Markup, for example, is unrelated to pluralization. But @jldec this will be your decision now. Being compatible with full MF2 from the get-go is in wrong IMO. The spec will change and we have different requirements because we have apps.

What is required for pluralization?

  • MessageBundle
  • Variants
  • Pluralization function

Seems not to be required:

  • Markup
  • Other functions (like date formatting, gendering etc.)
  • local declarations

Copy link
Member

Choose a reason for hiding this comment

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

Sync notes https://linear.app/opral/issue/MESDK-77/requirements-for-gender-and-plurals#comment-0dd412be.

It seems like local declarations can be removed if we go the bottoms up approach. cc @martin-lysk

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Removed for now.


export type VariableReference = Static<typeof VariableReference>
export const VariableReference = Type.Object({
type: Type.Literal("variable"),
Copy link
Member

Choose a reason for hiding this comment

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

Mismatch between type name and type string VariableReference vs variable. Recommendation, follow MessageFormat PascalCase for all types:

-variable
+VariableRefernece

Copy link
Collaborator

@LorisSigrist LorisSigrist May 6, 2024

Choose a reason for hiding this comment

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

The string "variable" is from the MessageFormat 2.0 Data-model recommendation. Should we deviate?
https://github.com/unicode-org/message-format-wg/blob/6d7b4ba213e686ff2d403d3025d38d76b42b75f7/spec/data-model/README.md#expressions

Copy link
Member

Choose a reason for hiding this comment

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

Oh wow, they deviated from the previous pattern then. I let @jldec decide. I prefer written out types it's a naming thing though which no real impact

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's keep this same as MF2 data model for now.

Comment on lines 10 to 17
/**
* A (text) element that is translatable and rendered to the UI.
*/
export type Text = Static<typeof Text>
export const Text = Type.Object({
type: Type.Literal("text"),
value: Type.String(),
})
Copy link
Member

Choose a reason for hiding this comment

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

MessageFormat 2.0 seems to have removed the Text type in favor of a pure string. What was their reasoning?

Notes from me:

  • iterating through pattern elements seems much easier if every node has node.type property instead of using union matching e.g. if (typeof node === string || node.type === XYZ)

@jldec
Copy link
Contributor Author

jldec commented May 6, 2024

Wondering if releasing a v2 of @inlang/message isn't better. Apps using the SDK now have name collisions:

import { Message } from "@inlang/sdk"

What message is imported here?

Recommendation:

  • Release V2 prerelease of @inlang/message instead of having a new package
  • Suffix all types with V2 e.g. MessageV2, PatternV2, etc. to avoid name collisions

(We will run into this problem again with a potential v3. Seems easier to then again suffix MessageV3 instead of having a new package and 3 potential name collisions)

@samuelstroschein that' s a good catch!

I was hoping that the new Message type would automatically be namespaced under the MessageBundle module but this is not the pattern that @inlang/sdk uses here

ideas:

  1. follow your suggestion of new suffixed types like MessageV2 etc.
  2. export new types like Message and MessageBundle ONLY as part of the project.query2 api, as proposed in MESDK-83
  3. differentiate Message from {namespace}.Message as proposed originally with AST.Message as the prefix.
  4. deprecate the @inlang/sdk root level types, and import new types only from versioned-interface packages E.g. import { Message } from @inlang/message-bundle
  5. namespace under sdk/* e.g. import { Message } from @inlang/sdk/message-bundle

@samuelstroschein
Copy link
Member

@jldec

  1. i propose to suffix the new interfaces with V2 and remove the suffix once we release the SDK in a new major version similar to sdk api for MessageBundle with variants inlang-sdk#45 (comment)
  2. https://linear.app/opral/issue/MESDK-84/get-rid-of-versioned-interface-packages triggered by this discussion

@jldec
Copy link
Contributor Author

jldec commented May 7, 2024

@martin-lysk any opinion on this?

@jldec
Copy link
Contributor Author

jldec commented May 7, 2024

@samuelstroschein ok - i'll add MessageBundleV2 and MessageV2 to sdk.
(no need for a new versioned-interface package, right)

@samuelstroschein
Copy link
Member

@jldec (no need for a new versioned-interface package, right)

correct.

@jldec
Copy link
Contributor Author

jldec commented May 9, 2024

@samuelstroschein
I have moved the new types into @inlang/sdk/v2 but kept the type names unchanged.
To avoid conflicts, it's easy to alias the imported type name individually, or import * as V2
The types are not exported at the root of the sdk.

@@ -0,0 +1,122 @@
import { LanguageTag as Locale } from "@inlang/language-tag"
Copy link
Member

Choose a reason for hiding this comment

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

You can pull the Locale type into the types here.

  • language tag has been it's own package to allow the community to use it
    -- this is not happening
  • eases getting rid of versioned-interfaces

@jldec jldec changed the title add @inlang/message-bundle Introduce @inlang/sdk/v2 MessageBundle and related types May 9, 2024
@jldec jldec changed the title Introduce @inlang/sdk/v2 MessageBundle and related types MESDK-77 add @inlang/sdk/v2 with MessageBundle and related types May 9, 2024
@jldec jldec merged commit 04fa4c5 into main May 9, 2024
3 checks passed
@jldec jldec deleted the message-bundle branch May 9, 2024 02:06
@github-actions github-actions bot locked and limited conversation to collaborators May 9, 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.

Requirements for gender and plurals
5 participants