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

Initial decomposition pass on matter.js #1154

Merged
merged 4 commits into from
Sep 9, 2024

Conversation

lauckhart
Copy link
Collaborator

This is a partial pass deconstructing the matter.js package. I tried to stick to decisions we'd previously agreed to.

The new @project-chip/matter.js-general package contains those portions of matter.js that are not Matter specific.

The new @project-chip/matter.js-model package contains the Matter object model.

Since I was moving stuff around I got rid of the Uint8Array monkey patches we did in ByteArray. For the most part this just consisted of replacing the "ByteArray" type with "Uint8Array". The methods we injected I moved to a "Bytes" namespace containing utility functions. This is a breaking change.

Following new conventions, the new packages don't have "matter.js" in their names, so they're just in "packages/general" and "packages/model". I also renamed packages/matter.js-tools to packages/tools. I did not rename the other directories yet.

I updated codegen and the intermediate model package dependencies, so codegen errors do not prevent codegen from running except for the generated model files in @project-chip/matter.js-model.

The new packages are smaller so I moved to a flat namespace for exports. So there are no subpaths exported from the "general" package, even though most of the exports were previously exported as subpaths of matter.js. I think this improves readability and makes consumption simpler.

"model" also has a flat namespace but this was true of the previous "@project-chip/matter.js/model" module so this isn't as big of a change.

I did my best to add exports to matter.js to support backward compatibility. However I used a little editorial control to select which exports to reproduce at the old location... I made sure to cover all of those we used previously in the examples. Many of the other exports were probably not ever used externally. Shouldn't be a big deal if I got it wrong; we can add additional exports or just inform users of the new locations.

Fixes #156

Copy link

Review changes with SemanticDiff.

packages/general/src/crypto/Key.ts Fixed Show fixed Hide fixed
packages/general/src/crypto/Key.ts Fixed Show fixed Hide fixed
@lauckhart lauckhart force-pushed the extract-general-and-model branch 4 times, most recently from e06d6e6 to 381dcd2 Compare September 9, 2024 00:59
@lauckhart
Copy link
Collaborator Author

Rebased onto #1172

@lauckhart lauckhart mentioned this pull request Sep 9, 2024
@lauckhart lauckhart force-pushed the extract-general-and-model branch 2 times, most recently from 9f31d80 to 21b9d1f Compare September 9, 2024 06:53
Copy link
Collaborator

@Apollon77 Apollon77 left a comment

Choose a reason for hiding this comment

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

beside the small stuff found I just do not like the "multiple full -general package exports in node package" ... lets simply kill the re-exports on matter-node not and just export the new functionality. These reexports are "deprecated" anyway since some versions and with adding peer-deps in second step we move further .. so time now.

The more "on purpose exports" in matter.js are same-ish ... I have no idea how we want to manage that on this detail level in the future ... that wold be the ly idea to sub-structure "general" exports too "by dir" to have that easier ...

packages/general/README.md Outdated Show resolved Hide resolved
packages/matter-node.js/src/exports/model.ts Show resolved Hide resolved
@Apollon77
Copy link
Collaborator

PS: And add infos to changelog please :-)

This is a partial pass deconstructing the matter.js package.  I tried to stick to decisions we'd previously agreed to.

The new @project-chip/matter.js-general package contains those portions of matter.js that are not Matter specific.

The new @project-chip/matter.js-model package contains the Matter object model.

Since I was moving stuff around I got rid of the Uint8Array monkey patches we did in ByteArray.  For the most part this just consisted of replacing the "ByteArray" type with "Uint8Array".  The methods we injected I moved to a "Bytes" namespace containing utility functions.  This is a breaking change.

Following new conventions, the new packages don't have "matter.js" in their names, so they're just in "packages/general" and "packages/model".  I also renamed packages/matter.js-tools to packages/tools.  I did not rename the other directories yet.

I updated codegen and the intermediate model package dependencies, so codegen errors do not prevent codegen from running except for the generated model files in @project-chip/matter.js-model.

The new packages are smaller so I moved to a flat namespace for exports.  So there are no subpaths exported from the "general" package, even though most of the exports were previously exported as subpaths of matter.js.  I think this improves readability and makes consumption simpler.

"model" also has a flat namespace but this was true of the previous "@project-chip/matter.js/model" module so this isn't as big of a change.

I did my best to add exports to matter.js to support backward compatibility.  However I used a little editorial control to select which exports to reproduce at the old location...  I made sure to cover all of those we used previously in the examples.  Many of the other exports were probably not ever used externally.  Shouldn't be a big deal if I got it wrong; we can add additional exports or just inform users of the new locations.

Fixes project-chip#156
@lauckhart
Copy link
Collaborator Author

PS: And add infos to changelog please :-)

Those are in the original commit, LMK if you think it needs more

@lauckhart
Copy link
Collaborator Author

beside the small stuff found I just do not like the "multiple full -general package exports in node package" ... lets simply kill the re-exports on matter-node not and just export the new functionality. These reexports are "deprecated" anyway since some versions and with adding peer-deps in second step we move further .. so time now.

I'll do this in a separate commit refactoring matter-node.js

@lauckhart
Copy link
Collaborator Author

The more "on purpose exports" in matter.js are same-ish ... I have no idea how we want to manage that on this detail level in the future ... that wold be the ly idea to sub-structure "general" exports too "by dir" to have that easier ...

These are a one-time backward-compat thing. There are also changes in both directory layout and exported identifier names so they need a little finagling beyond straight reexports (e.g. a few files from "common" are now in general/src/util but most aren't). Exporting the directories would make it slightly easier but I wanted to avoid because there wouldn't be any way to prevent people from using them.

Also changes git package versions from "git" to "0.0.0-git" because NPM intermittently chokes on the former.
Analyze TS files as a single project and do not analyze intermediate models.

Linting goes from 4m 16s to 1m 49s on my laptop.  Hopefully further optimization possible.
@lauckhart lauckhart merged commit 279941b into project-chip:main Sep 9, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trivial: ByteArray thoughts
2 participants