-
Notifications
You must be signed in to change notification settings - Fork 418
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
Introduce a Debug mode #354
Conversation
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.
Thanks @adrienpoly for implementing this.
No more console.log
in connect()
😉
Just a couple of small comments.
Co-authored-by: dbo <dimitribosch@hotmail.com>
Thanks @adrienpoly for this work, can't wait to have a logger too! Typos in HTML resulting in not having the Stimulus Controller loaded happen, on dev habbit counter-measure has been 🤔 I wonder about the default though. From your description, it seems that we'd have to manually add that to our pack: application.debug = process.env.NODE_ENV === 'development' This opt-in approach is good for retro-compatibility, but in the meantime a lot of teams upgrading Stimulus would miss that feature. What would be the sensible default? I'm leaning towards having that line within the library, while still having the possibility to force no debugging in the application code with: application.debug = false |
process.env.NODE_ENV === 'development' @ssaunier I think the ENV variable work when you have a build system in place such as webpack but would not work if you are using the UMD version of Stimulus or the new autoloading approach from https://github.com/hotwired/stimulus-rails with Skypack. So unless we would have a per build type default value for the debugger I think we need to default it to false. |
btw, that makes me think of something for v2 of this feature: wouldn't it be cool to log when a |
This looks incredible! Thank you @adrienpoly (I actually searched for this after seeing your debug mode over on stimulus-use (https://stimulus-use.github.io/stimulus-use/#/docs/debug). I know from personal experience that this is rather annoying, but does anyone know if there's a plan/timeline to merge some PR's like this and create a 2.1 release? I'm working on a tutorial for Stimulus for the Symfony world and this feature, in particular, would be great to show off. Also: is there anything we know blocking the PR? Missing tests? Thanks! |
While we are waiting for the PR to be merged, not knowing what's preventing the merge, I tried the suggestion https://stimulus-use.github.io/stimulus-use/#/docs/debug and couldn't get the |
@daya StimulusUse debug mode is only for some StimulusUse mixins (by example useDistpactch, useVisibility some others are still in progress). It does noyt cover all of the standard lifecycle events of Stimulus. This PR does To the maintainers |
This is great @adrienpoly. I'd like to follow this up with a default assignment of the stimulus application instance to window, just like we do with Turbo now, such that it's easy to toggle on the debug mode. |
Please reconsider adding Stimulus to the global window namespace by default, as there's no reason to force this on people who don't need/want it. Anyone that does want it can add window.Stimulus = application to their |
This would make it easier to debug Stimulus both as a way to turn on/off logging, including in production, but also to inspect the state of registered controllers. Do you have any cases where assigning stimulus to window would cause problems? Of course, even if we did move to doing this by default, one could also just add window.Stimulus = null to clear it out. |
I understand the functionality and the mechanics. I also think this PR is a nice enhancement. My comment is specifically in regards to adding global variables that aren't necessary. I don't have anything to add beyond what I've said. My previous comment stands on its own merits. A compromise might be to add the assignment to the standard |
Good point, @leastbad. I've updated both the Stimulus handbook and the default generated index.js in stimulus-rails to use the window.Stimulus assignment. Then it's easy enough for someone to change to use a local variable, if they don't think they benefit from the debugging benefits of the global 👍 |
👍 for merging this PR about the window assignment I will do more tests but potentially it could cause issues in app where multiple packs are loaded (each pack having Stimulus). With the assignment to |
Yeah, that's why we're better off just doing the default setup as generated code, and then people can change that if they have unique situations like that. |
This PR adds a debug mode to Stimulus and close #349
In debug mode application lifecycles, controller lifecycles and actions events are sent to the Stimulus
Logger
with a default output to the console. Each log includes context information available in a detailed view.Example:
Usage
debug mode can be enabled during the Stimulus application initialization phase
Or even at run time as suggested here if
application
is accessible at the window level.Message content & styling
Content
Messages are collapsed groups. Title of the group follow the pattern
emitter #action
The emitter can be
application
or acontroller identifier
Examples:
application #staring
application #stared
form #initialize
form #connect
form #submit
-> actionOpening the group provides a context object with details:
Style
I applied a few colors with a variant for dark mode. I took Purple and Yellow from Basecamp website. Matter of taste and easily changeable.
Values
Value changes are not logged at this point. It shouldn't be difficult to add if needed.
Namespacing
No particular namespace is applied to the log message at this point. This is a subject of discussion. Do we want something like :
Stimulus - emitter #action
for the collapsed group title ?Custom logger
By default, the logger is the
console
. A customLogger
can be supplyTests
This PR does not include tests at this point. I am not sure how you would like this to be tested. One approach I could think of is to create a custom Logger that would collect messages (could store them in the
application
). And then do some assertion on the amount of messages and type we receive. An approach a bit similar to thelog_controller_test_case
but at the application level this time.