Skip to content

feat: Hide more internal functionality under symbols #1387

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

Merged
merged 4 commits into from
Jun 23, 2025
Merged

Conversation

iwoplaza
Copy link
Collaborator

No description provided.

Copy link

github-actions bot commented Jun 20, 2025

pkg.pr.new

packages

pnpm i https://pkg.pr.new/software-mansion/TypeGPU/typegpu@1387
pnpm i https://pkg.pr.new/software-mansion/TypeGPU/typegpu@1b8a6f87648b830cfd26b7375fcf3c07e04a37dc

benchmark
view benchmark

commit
view commit

@iwoplaza iwoplaza marked this pull request as ready for review June 20, 2025 13:15
Copy link
Contributor

@reczkok reczkok left a comment

Choose a reason for hiding this comment

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

🧹🧼

export const $gpuRepr = Symbol(`typegpu:${version}:$gpuRepr`);
/**
* Type token for the inferred partial representation of a resource.
* If present, it shadows the value of `$repr`
Copy link
Contributor

Choose a reason for hiding this comment

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

Only for partial IO

@aleksanderkatan
Copy link
Contributor

I'm still not really convinced on using so many symbols. As we discussed earlier, they don't really provide privacy, the only advantage I see is that there are no possible name collisions. Why not hide these potential props behind [$internal] instead of creating a new symbol for each one?
Could we include some UUIDs in these symbols like I mentioned in #1294 so that multiple imports are easier to debug?

@iwoplaza
Copy link
Collaborator Author

I'm still not really convinced on using so many symbols. As we discussed earlier, they don't really provide privacy, the only advantage I see is that there are no possible name collisions. Why not hide these potential props behind [$internal] instead of creating a new symbol for each one?

I was leaning towards creating a unique symbol for each, rather than putting them under $internal, so that it's:

  1. Easier to track what their purpose is (they can hold documentation, which does not have to be copied at each use site),
  2. Easier to find their uses in the codebase.
  3. We don't have to fear of accidentally using the same name for a different purpose across objects. This was the case for $internal.dataType, which meant different things for different resources, causing unnecessarily complicated and bug prone resolution logic.

Could we include some UUIDs in these symbols like I mentioned in #1294 so that multiple imports are easier to debug?

That's definitely something we could do 👍. But maybe as part of a separate PR.

@iwoplaza iwoplaza merged commit 63a2247 into main Jun 23, 2025
6 checks passed
@iwoplaza iwoplaza deleted the fix/more-symbols branch June 23, 2025 11:07
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.

3 participants