fix(core): imported symbols runtime leak #2896
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- also check that they are not exposed in d.ts
- also check that they are not exposed in d.ts
- also check that they are not exposed in d.ts
- also check that they are not exposed in d.ts
- also check that they are not exposed in d.ts
idoros
added
bug
barak007
reviewed
Sep 12, 2023
barak007
reviewed
Sep 12, 2023
…-runtime-symbols-scope
barak007
approved these changes
Sep 18, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request addresses a longstanding regression related to the unintended runtime leakage of imported symbols.
During the process of refactoring the internal symbol resolution code to achieve consistent handling of various symbols, an oversight occurred. This oversight caused the generated JavaScript runtime and the
.d.tsfiles to inadvertently expose imported symbols of the stylesheet.This situation is problematic because Stylable's assumption is that symbols of a stylesheet are only meant to be used if the stylesheet is imported into a JavaScript file. If it's not imported this way, it's assumed that the symbols' namespacing can't be connected to the DOM and can thus be removed as unnecessary code from the final output.
This fix is undoubtedly necessary. However, it's important to acknowledge that there's a possibility it could introduce disruptions in external code.
To mitigate this potential impact, we're planning to conduct a thorough search across both public code repositories and internal code projects accessible to us. This search aims to identify instances where this fix might cause conflicts or complications. Based on our findings, we will make an informed decision about whether to integrate this fix into the upcoming v5 release or reserve it for inclusion in the subsequent major release, v6.For now this change will only effect version 6, and we might port it back to v5 at some point.