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

Make TS types consistent with the documentation terminology #162

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

mathfox
Copy link

@mathfox mathfox commented Dec 9, 2024

This PR comes as a solution to the problem that I faced when I was not able to migrate my project from the 0.3.2 version to the 0.4.1-rc.0 one.
The reason was that the latest type definitions break the whole project with an unclear errors.

I believe that the changes are based off the terminology used in the jecs/flecs documentation.

I breakdown the entity type into 3 variations:

  • Id (the base type);
  • Tag (extends Id, allows the entity to be used as a tag);
  • Entity (extends Tag, allows the entity to be used as a component);

This PR also adds a missing 'world' type parameter in the type definition for 'pair_first' and 'pair_second' functions.

Fixes #161

@Ukendio
Copy link
Owner

Ukendio commented Dec 9, 2024

I don't think this PR is necessary, have you tried installing the package via npm i git+https://github.com/ukendio/jecs.git

@mathfox
Copy link
Author

mathfox commented Dec 9, 2024

Well, it was necessary for me to migrate from the previous version with a very few errors showing up. I don't think that the installation part is relevant in my case (either version breaks the whole project with a bunch of unclear errors). I believe I got the terminology right in this PR, correct me if I'm wrong. I think there should not exist a Pair type when at it's core it's just an entity (or id) that could be used as a component/tag only (although I did not restrict the usage of 'add'/'set' methods on this kind of entities in this PR).

@Ukendio
Copy link
Owner

Ukendio commented Dec 10, 2024

I am mostly referring to the types which appear to me to be ill formed. @EncodedVenom can review it as well

@EncodedVenom
Copy link
Collaborator

EncodedVenom commented Dec 19, 2024

Upon first glance I don't like some of the renaming done, but I'll come back tomorrow with a more detailed analysis on this to see what needs some work.

@EncodedVenom
Copy link
Collaborator

Upon further review, it seems the whole reason this PR was made was to fix the issue listed in #161 which stems from an ill-defined entity type fixed in this commit as found via several users in the jecs thread on the ROSS server. I think this is beyond the scope of the original issue and should be split off into a new one. I will not make that call and let @Ukendio handle that one.

Since this is not a huge problem right now and mostly stems from minor type issues, I think we can afford to sit on this for a while.

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.

Latest TS type definitions break the whole project with unclear errors when migrating from 0.3.2 to 0.4.1-rc.0
3 participants