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

[WIP] E2E tests #218

Merged
merged 12 commits into from
Feb 1, 2024
Merged

[WIP] E2E tests #218

merged 12 commits into from
Feb 1, 2024

Conversation

NyxCode
Copy link
Collaborator

@NyxCode NyxCode commented Jan 31, 2024

This PR adds the directory e2e, which is supposed to include end-to-end tests to make sure imports / exports are working.

I added these two tests to illustrate how the current default behaviour is somewhat broken:

dependencies

A user creates the crate consumer, which depends on some crate dependency1, which possibly lives on crates.io.
If a struct in consumer contains a type from dependency1, it should be exported as well.

workspace

A user creates a workspace, containing crate1, crate2, and parent.
crate1 and crate2 are independent, but parent depends on both crate1 and crate2.

CI runs cargo test and tsc bindings/* to check if the output is at least valid, which it currently is not for both tests.

@NyxCode
Copy link
Collaborator Author

NyxCode commented Jan 31, 2024

@escritorio-gustavo I think this is the bigger picture we have to keep in mind. Both screnarios which are tested here should just work out-of-the box!

Comment on lines +3 to +6
#[derive(TS)]
pub struct LibraryType {
pub a: i32
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, since this type doesn't contain #[export] we'd have to come up with some other way to export it in the consumer crate

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right! I'm not sure about how to exactly solve this yet - maybe a #[ts(export)] type should cause all of its dependencies to be exported as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

We'd have to make sure it's only exported once, even if the same type is used in a bunch of places

Copy link
Contributor

Choose a reason for hiding this comment

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

But this might be confusing, as the user is not explicitly #[ts(export)]ing the type

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But this might be confusing, as the user is not explicitly #[ts(export)]ing the type

The problem in that scenario is that the user just cant explicitly export it, since it's in an other crate that may not be under his control.

We'd have to make sure it's only exported once, even if the same type is used in a bunch of places

That's true, this might be the biggest challenge here.

Copy link
Contributor

@escritorio-gustavo escritorio-gustavo Jan 31, 2024

Choose a reason for hiding this comment

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

The problem in that scenario is that the user just cant explicitly export it, since it's in an other crate that may not be under his control.

That is true, and it brings up the question I asked in #211:

Should library code in crates.io ever implement TS? Seems like this should be an application code decision (especially the extra dependency + tests)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(let's maybe have this discussion here then)

Should library code in crates.io ever implement TS? Seems like this should be an application code decision (especially the extra dependency + tests)

I think it'd be a good idea to support that, yeah! These libraries would probably never #[ts(export)], but making their types implement TS might be usefull in some cases, I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is true, and it brings up the question I asked in #211

Speaking of which, this is getting hard to follow across two PRs, I opened a discussion (#219) to centralize this

Comment on lines 3 to 6
#[derive(TS)]
pub struct Crate2 {
pub x: i32
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe I've got this to work when #[export, export_to = "..."] is used

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, if all crates of the workspace use #[ts(export_to = "../some_dir")], then they all put their files in one directory & the imports work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct, and since they are all in the same workspace, it should be easy enough to ensure they are in the same directory.
Although, due to the changes to path diffing, I don't think they even need to be... just #[ts(export)] might be enough

Copy link
Contributor

Choose a reason for hiding this comment

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

just #[ts(export)] might be enough

Haven't tested this yet though

Copy link
Contributor

Choose a reason for hiding this comment

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

Haven't tested this yet though

Tested it now, it works!

Introduce TypeList, always export Dependencies as well
@escritorio-gustavo escritorio-gustavo marked this pull request as ready for review February 1, 2024 20:02
@escritorio-gustavo escritorio-gustavo merged commit 5db57ab into main Feb 1, 2024
8 checks passed
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.

2 participants