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

More FFI APIs #178

Merged
merged 11 commits into from
Jan 26, 2025
Merged

More FFI APIs #178

merged 11 commits into from
Jan 26, 2025

Conversation

Manishearth
Copy link
Contributor

Unfortunately the temporal_core stuff breaks all the duration APIs, and I don't understand what it's doing. I wrote this PR before that change, help updating it to work with Duration appreciated.

In general I don't think it's a good idea to have two separate Duration types with differing APIs. Opting in to features might enable new APIs, but it's not kosher to disable APIs that way, which seems to be the case here.

@nekevss
Copy link
Member

nekevss commented Jan 22, 2025

I'll take a look at it ASAP

@Manishearth
Copy link
Contributor Author

useful bit of context: I wrote this on the plane and rebased it later

@nekevss
Copy link
Member

nekevss commented Jan 22, 2025

The quickest way to fix this issue is to opt out temporal_capi from the default features or remove the default feature from temporal_rs entirely and have the "full" feature be opt-in.

But as mentioned in #165. The wrapper concept around core might need to be looked at again.

@Manishearth
Copy link
Contributor Author

@nekevss I don't think opting in to a feature should ever break code?

@Manishearth
Copy link
Contributor Author

Manishearth commented Jan 22, 2025

For example, I'm having code break because YearMonth uses the core duration. From what I can tell this core vs builtin split doesn't quite work: Core contains types which reference each other, and only some of them are wrapped in builtin and reexported, so you get a broken mix of things.

error[E0308]: mismatched types
   --> temporal_capi/src/plain_year_month.rs:108:36
    |
108 |                 .subtract_duration(&duration.0, overflow.into())
    |                  ----------------- ^^^^^^^^^^^ expected `Duration`, found `temporal_rs::Duration`
    |                  |
    |                  arguments to this method are incorrect
    |
    = note: expected reference `&temporal_rs::temporal_core::Duration`
               found reference `&temporal_rs::Duration`
note: method defined here
   --> /home/manishearth/dev/temporal/src/builtins/core/year_month.rs:139:12
    |
139 |     pub fn subtract_duration(
    |            ^^^^^^^^^^^^^^^^^

@Manishearth
Copy link
Contributor Author

I've done the fix to make this landable, but this situation isn't ideal.

@nekevss nekevss added C-enhancement New feature or request C-FFI Changes related to FFI labels Jan 24, 2025
@nekevss nekevss merged commit 1501955 into boa-dev:main Jan 26, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement New feature or request C-FFI Changes related to FFI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants