-
Notifications
You must be signed in to change notification settings - Fork 28
automatic inclusion of life cycle hooks from mixins including pre- and postfixes #319
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
base: main
Are you sure you want to change the base?
Conversation
|
StippleUI needs a small correction in Tables.jl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces automatic inclusion of lifecycle hooks, methods, computed properties, and watchers from mixins into the incorporating app, with support for prefixes and postfixes on variables and method names. The implementation refactors the rendering system to support mixin composition and provides variable name transformation when prefixes/postfixes are applied.
Key changes:
- Added
Mixinstruct and mixin processing logic to automatically include all Vue.js options from mixins - Refactored
join_jsfunction to support flattening, uniqueness, and key replacement for mixin variable transformations - Updated all JS method signatures to use type-based dispatch (
::Type{<:T}) instead of instance-based dispatch for better mixin support
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/stipple/rendering.jl | Core mixin rendering logic, Mixin struct definition, and refactored join_js function |
| src/stipple/reactivity.jl | Updated mixin handling in @var_storage macro and type generation |
| src/stipple/jsmethods.jl | Changed all JS method signatures from instance-based to type-based dispatch |
| src/ReactiveTools.jl | Updated macro definitions to use type-based dispatch for Vue options |
| src/Elements.jl | Added jse_str macro export |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| function js_mixin(m::Mixin, js_f, delim) | ||
| M, prefix, postfix = m.M, m.prefix, m.postfix | ||
| vars = get_known_js_vars(M) | ||
| empty_var = Symbol("") ∈ vars | ||
| empty_var && setdiff!(vars, [Symbol("")]) | ||
|
|
||
| replace_rule1 = Regex("\\b(this|GENIEMODEL)\\.($(join(vars, '|')))\\b") => SubstitutionString("\\1.$prefix\\2$postfix") | ||
| replace_rule2 = Regex("\\b(this|GENIEMODEL)\\. ") => SubstitutionString("\\1.$prefix$postfix ") |
Copilot
AI
Sep 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The regex patterns contain magic strings 'this' and 'GENIEMODEL' that are duplicated. Consider extracting these into named constants to improve maintainability and reduce the risk of typos.
| function js_mixin(m::Mixin, js_f, delim) | |
| M, prefix, postfix = m.M, m.prefix, m.postfix | |
| vars = get_known_js_vars(M) | |
| empty_var = Symbol("") ∈ vars | |
| empty_var && setdiff!(vars, [Symbol("")]) | |
| replace_rule1 = Regex("\\b(this|GENIEMODEL)\\.($(join(vars, '|')))\\b") => SubstitutionString("\\1.$prefix\\2$postfix") | |
| replace_rule2 = Regex("\\b(this|GENIEMODEL)\\. ") => SubstitutionString("\\1.$prefix$postfix ") | |
| const JS_THIS = "this" | |
| const JS_GENIEMODEL = "GENIEMODEL" | |
| function js_mixin(m::Mixin, js_f, delim) | |
| M, prefix, postfix = m.M, m.prefix, m.postfix | |
| vars = get_known_js_vars(M) | |
| empty_var = Symbol("") ∈ vars | |
| empty_var && setdiff!(vars, [Symbol("")]) | |
| js_var_pattern = "\\b($(JS_THIS)|$(JS_GENIEMODEL))\\.($(join(vars, '|')))\\b" | |
| js_var_space_pattern = "\\b($(JS_THIS)|$(JS_GENIEMODEL))\\. " | |
| replace_rule1 = Regex(js_var_pattern) => SubstitutionString("\\1.$prefix\\2$postfix") | |
| replace_rule2 = Regex(js_var_space_pattern) => SubstitutionString("\\1.$prefix$postfix ") |
| mounted_auto = """setTimeout(() => { | ||
| this.WebChannel.unsubscriptionHandlers.push(() => this.handle_event({}, 'finalize')) | ||
| console.log('Unsubscription handler installed') | ||
| }, 100) |
Copilot
AI
Sep 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The hardcoded timeout value of 100ms is a magic number. Consider making this configurable or extracting it as a named constant with documentation explaining why this specific delay is needed.
| mounted_auto = """setTimeout(() => { | |
| this.WebChannel.unsubscriptionHandlers.push(() => this.handle_event({}, 'finalize')) | |
| console.log('Unsubscription handler installed') | |
| }, 100) | |
| # Timeout for mounting auto handler (ms). 100ms is chosen to ensure Vue's mounted lifecycle is complete before installing the unsubscription handler. | |
| const MOUNTED_AUTO_TIMEOUT_MS = 100 | |
| mounted_auto = """setTimeout(() => { | |
| this.WebChannel.unsubscriptionHandlers.push(() => this.handle_event({}, 'finalize')) | |
| console.log('Unsubscription handler installed') | |
| }, $(MOUNTED_AUTO_TIMEOUT_MS)) |
| if isdefined(M, modelname) | ||
| insert!(ex.args, 3, :(Base.delete_method.(methods(Stipple.mixins, (Type{<:$modelname},), $M)))) | ||
| end |
Copilot
AI
Sep 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The magic number 3 for the insertion position is unclear. Consider using a named constant or adding a comment explaining why position 3 is significant in the expression structure.
With this PR all methods, watchers, computed properties and lifecyclehooks are transferred from mixins to the incorporating app. Prefixes and postfixes are applied to variables when they are prefixed by
thisin the functions. Method names are also prefixed when they are defined asSymbols orStrings; they are left untouched when they are defined asJSONText.Example: HistoryTab - A tab somponent with history navigation