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

Partial support for duplicated symbols #143

Merged
merged 2 commits into from
Oct 25, 2024

Conversation

jhugman
Copy link
Owner

@jhugman jhugman commented Oct 24, 2024

 According to The Big O of Code Reviews, this is a O(n) change.

This PR partially fixes #68:

  • it re-exports each named crate as default export.
  • it documents working with megazords.
  • including the limitations of the Swift backend.

The second part to fix #68 is a little more involved, but is not yet worth it:

  • for imported types and their converters, the bindings should use a more fully qualified name. e.g.

Instead of:

import { Crate1 } from "./crate1";
import crate1 from "./crate1";

const { FfiConverterTypeCrate1 } = crate1.converters;

// In use
type MyType = {
  scalar: Crate1,
  optional: Crate1 | undefined,
  list: Array<Crate1>,
}

// FfiConverterTypeCrate1 being used.
const lift = FfiConverterTypeCrate1.lift.bind(FfiConverterTypeCrate1);

The next part needs to generate code:

import * as crate1 from "./crate1";

// In use
type MyType = {
  scalar: crate1.Crate1,
  optional: crate1.Crate1 | undefined,
  list: Array<crate1.Crate1>,
}

// FfiConverterTypeCrate1 being used.
const lift = crate1.converters.FfiConverterTypeCrate1.lift.bind(crate1.converters.FfiConverterTypeCrate1);

I think I will file a new issue with this in, and close #68.

This PR partially fixes #68:

- it re-exports each named crate as default export.
- it documents working with megazords.
- including the limitations of the Swift backend.

The second part to fix #68 is a little more involved, but is not yet worth it:

- for imported types and their converters, the bindings should use a more
fully qualified name. e.g.

Instead of:

```ts
import { Crate1 } from "./crate1";
import crate1 from "./crate1";

const { FfiConverterTypeCrate1 } = crate1.converters;

// In use
type MyType = {
  scalar: Crate1,
  optional: Crate1 | undefined,
  list: Array<Crate1>,
}

// FfiConverterTypeCrate1 being used.
const lift = FfiConverterTypeCrate1.lift.bind(FfiConverterTypeCrate1);
```

The next part needs to generate code:

```ts
import * as crate1 from "./crate1";

// In use
type MyType = {
  scalar: crate1.Crate1,
  optional: crate1.Crate1 | undefined,
  list: Array<crate1.Crate1>,
}

// FfiConverterTypeCrate1 being used.
const lift = crate1.converters.FfiConverterTypeCrate1.lift.bind(crate1.converters.FfiConverterTypeCrate1);
```

I think I will file a new issue with this in, and close #68.
Copy link
Collaborator

@Johennes Johennes left a comment

Choose a reason for hiding this comment

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

Reviewed and tested with https://github.com/unomed-dev/react-native-matrix-sdk and I think this looks good. Here are the changes it caused for reference:

diff --git a/src/index.tsx b/src/index.tsx
index 242ba02..36133d2 100644
--- a/src/index.tsx
+++ b/src/index.tsx
@@ -12,17 +12,31 @@ export * from './generated/matrix_sdk_crypto';
 export * from './generated/matrix_sdk_ffi';
 export * from './generated/matrix_sdk_ui';
 
+// Now import the bindings so we can:
+// - intialize them
+// - export them as namespaced objects as the default export.
+import * as matrix_sdk from './generated/matrix_sdk';
+import * as matrix_sdk_base from './generated/matrix_sdk_base';
+import * as matrix_sdk_common from './generated/matrix_sdk_common';
+import * as matrix_sdk_crypto from './generated/matrix_sdk_crypto';
+import * as matrix_sdk_ffi from './generated/matrix_sdk_ffi';
+import * as matrix_sdk_ui from './generated/matrix_sdk_ui';
+
 // Initialize the generated bindings: mostly checksums, but also callbacks.
-import matrix_sdk_ from './generated/matrix_sdk';
-import matrix_sdk_base_ from './generated/matrix_sdk_base';
-import matrix_sdk_common_ from './generated/matrix_sdk_common';
-import matrix_sdk_crypto_ from './generated/matrix_sdk_crypto';
-import matrix_sdk_ffi_ from './generated/matrix_sdk_ffi';
-import matrix_sdk_ui_ from './generated/matrix_sdk_ui';
+matrix_sdk.default.initialize();
+matrix_sdk_base.default.initialize();
+matrix_sdk_common.default.initialize();
+matrix_sdk_crypto.default.initialize();
+matrix_sdk_ffi.default.initialize();
+matrix_sdk_ui.default.initialize();
+
+// Export the crates as individually namespaced objects.
+export default {
+  matrix_sdk,
+  matrix_sdk_base,
+  matrix_sdk_common,
+  matrix_sdk_crypto,
+  matrix_sdk_ffi,
+  matrix_sdk_ui,
+};
 
-matrix_sdk_.initialize();
-matrix_sdk_base_.initialize();
-matrix_sdk_common_.initialize();
-matrix_sdk_crypto_.initialize();
-matrix_sdk_ffi_.initialize();
-matrix_sdk_ui_.initialize();

docs/src/guides/megazords.md Outdated Show resolved Hide resolved
```admonish warning title="Duplicated identifiers"
Due to Swift's large granular module sytem, crates in the same megazord cannot have types of the same name.

This may be solved in Swift at some point— e.g. by adding prefixes— but until then, duplicate identifiers will cause a Typescript compilation error as the types are smooshed together in `index.tsx`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
This may be solved in Swift at some point— e.g. by adding prefixes— but until then, duplicate identifiers will cause a Typescript compilation error as the types are smooshed together in `index.tsx`.
This may be solved in Swift at some point — e.g. by adding prefixes — but until then, duplicate identifiers will cause a Typescript compilation error as the types are smooshed together in `index.tsx`.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I have several em dashes throughout the documentation with this style.

Let's fix them all in one separate PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh ok. I wasn't aware it's a particular style, apologies. If that's the case, we can just keep it like this everywhere.

@jhugman jhugman merged commit c7bb0c3 into main Oct 25, 2024
5 checks passed
@jhugman jhugman deleted the jhugman/68-namespace-types-from-different-crates branch October 25, 2024 15:22
@jhugman
Copy link
Owner Author

jhugman commented Nov 2, 2024

Swift work seems to being tracked in this issue.

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.

Handle namespaced type references from other files/packages
2 participants