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

Architecture questions #18

Closed
jzaefferer opened this issue Feb 18, 2014 · 9 comments
Closed

Architecture questions #18

jzaefferer opened this issue Feb 18, 2014 · 9 comments

Comments

@jzaefferer
Copy link

I have some questions about the plans for architecture that the 0.1.0 blog post doesn't cover (or I missed it):

  • It looks like the existing plugins are all for transforming trees. Are there plans for validating trees, ala jshint et al? Those would take a tree as input, but don't have an output tree (just logs)
  • Also related, any plans for making plugins reusable or sharable between various tools? Specifically the node-task spec should be interesting here: https://github.com/node-task/spec/wiki - though based on the discussion in the blog post, I suspect that the proposed Record format will not be sufficient. Not super urgent, since the plans for node-task have been pushed to a later release.
  • Related to the above, what are plans for logging? Grunt does a reasonable job in this area, while Gulp, last I checked, had no logging. This is especially importing for plugins/tasks that validate their input like jshint. Currently each grunt task has to implement that logging from scratch, so there's definitely potential for abstracting.
@joliss
Copy link
Member

joliss commented Feb 18, 2014

Are there plans for validating trees, ala jshint et al?

It's possible to make a linting plugin that returns its input tree (so it's a no-op), but if it encounters linting errors it either prints a warning or fails the build.

In dev mode, it seems that failing on linting errors is not a good strategy - as I refactor or debug code, I'll often have intermediate code that doesn't pass JSHint but that I still want to run.

So I'm wondering if linting generally just belongs at the end of the test suite, and shouldn't be part of the build process.

If linting were done by a Broccoli plugin, we'd have the advantage that we can cache lint results quite easily, so we'd get constant-time (near-instantaneous) linting. JSHinting the Ember sources (87 KLOC) takes 10 seconds, so it can be a worthwhile optimization.

I'm really unsure.

Also related, any plans for making plugins reusable or sharable between various tools?

Maybe. Node-task and vinyl (which I believe derives from node-task) both use in-memory representations, which I'm not a fan of. That said, Broccoli's plugin API is quite simple, and it could conceivably be shared with other build systems, or you could build broccoli adapters for other build systems. I'm happy to coordinate with other build tool authors. (If necessary, we can even rename the plugin API spec to something that doesn't have "broccoli" in its name.)

Related to the above, what are plans for logging?

I don't have any plans for logging in Broccoli so far. (Of course you can do console.error or load your own logging library.) Can you elaborate what you need it for? And do you think it should live in Broccoli core?

@scottgonzalez
Copy link

Leaving logging completely up to the individual tasks makes things like standard logging vs. verbose logging difficult to manage. Verbose logging has been really helpful while developing and debugging complex tasks.

@stefanpenner
Copy link
Contributor

@joliss we have an interesting approach planned for logging in ember. That will be conducive to many packages and many addons. You may want to ping señor @wycats about it. It seems like it (or the idea) may work well in this scenario aswell.

@joliss
Copy link
Member

joliss commented Feb 19, 2014

Chat with @scottgonzalez. Re logging:

Jo Liss: Re https://github.com/joliss/broccoli/issues/18#issuecomment-35437425 , is this about developing tasks (plugins), or writing/debugging build definitions?
Scott González: Both, though for the latter it's really about tracing the state within the task for bad input, as opposed to bad build config.
Jo Liss: For plugin development, sprinkling console.error isn't good enough?
Scott González: No, this would be for actually logging all the steps of the task.
Jo Liss: For debugging build definitions, I wonder if we can actually expose a way to look at the intermediate files in Broccoli, via the browser.
Scott González: Let me give you a concrete example with actual code.
Jo Liss: cool
Scott González: https://github.com/scottgonzalez/grunt-wordpress/blob/master/tasks/wordpress/posts.js#L158
Scott González: Look for grunt.verbose in there.
Scott González: There's a lot of information that gets logged, but only if you ask for it.
Scott González: This is publishing content from local files to WordPress.
Scott González: Without the verbose flag, it still logs for every post, but it only logs that it was successful: https://github.com/scottgonzalez/grunt-wordpress/blob/master/tasks/wordpress/posts.js#L223
Scott González: This is a fairly complex grunt task, coming in around 1k LOC.
Jo Liss: hm nods
Scott González: And for the jQuery sites, this is just a portion of the build process.
Scott González: There are a few transforms: markdown -> HTML, syntax highlighting, publishing to WordPress
Jo Liss: if for debugging you'd just sprinkle console.log all over the place, would that be terribly painful?
Jo Liss: how much does making this a first class thing actually help?
Scott González: Well, it would either always be noisy, or everything would be commented out, making it annoying to use when you actually run into a problem.
Scott González: Making it first class ensures that there are conventions that task authors follow.
Jo Liss: hm. i'll think about it
Scott González: I think there's even a Grunt issue about having structured logs to get more consistent messaging across tasks.
Scott González: A quick search turned up gruntjs/grunt#697 and gruntjs/grunt#957
Scott González: One thing I had thought about was just having tasks use something like Bunyan since it already has a defined API with various logging levels and supports structured data.
Scott González: Then if the tasks just exposed the streams option that gets passed to the logger, the build tool could just hook into it for all tasks.
Scott González: I haven't actually spent any time seeing how well that would work.
Scott González: But it was a thought I had when I was thinking about how node-task should work (I don't actually think that project is going to work out).
Jo Liss: yes, so i sympathize with the general need for logging
Jo Liss: what makes me really hesitant is that i'd have to add it to the plugin api
...
Jo Liss: and in the spirit of node-task (after a conversation with tyler) i defined a very minimal plugin api, that can potentially be reused
...
Scott González: Maybe I'll try to play with some ideas for logging and see what it looks like.
Jo Liss: please do

Re linting:

Scott González: The linting issue is interesting as well.
Scott González: I agree that it's useful to have a build continue even after a linting error.
Scott González: But the idea of conditional failures based on the mode is tricky.
Jo Liss: yes; if we want linting as part of the build process, it seems that we have to think about logging lint warnings
Jo Liss: really?
Scott González: Well, the first question is how would you set the mode?
Scott González: Would it be based on whether you're doing continuous builds or would it be a flag?
Jo Liss: basically whether you're doing continuous builds
Jo Liss: https://github.com/joliss/broccoli-sample-app/blob/master/Broccolifile.js#L7
Jo Liss: queried like so: https://github.com/joliss/broccoli-sample-app/blob/master/Broccolifile.js#L63
Scott González: Ok, so you would just add another method to broccoli-env?
Jo Liss: what kind of other method?
Jo Liss: it seems that "warn if env == 'dev', fail if env == 'prod'" is good enough?
Scott González: That's probably fine.
Jo Liss: i'm still really unsure whether linting belongs in the build or the test
Scott González: We do a lot of lint-type tests in our build process.
Scott González: Even things like are your node modules up to date?
Scott González: We've had way too many issues with people not running npm install after git pull.
Scott González: And then the build runs with the wrong version of a dependency.
Jo Liss: right. seems like those are like tests that would run before the build or so
Scott González: Which may or may not cause an error in the build, but is likely to cause incorrect output.
Jo Liss: or after the build too
Scott González: As you're developing with broccoli serve running, wouldn't you want the linter to run, even if it's not causing build failures on error?
Jo Liss: well that's one way
Jo Liss: the alternative is that you'd have something like broccoli serve-and-test that rebuilds and then kicks off the test suite each time
Jo Liss: and jshint would be part of the test suite
Scott González: What about linters that should always fail a build?
Scott González: Like xmllint.
Scott González: Would those just be treated as part of the build instead of part of the test?
Jo Liss: those would more reasonably belong in the build than in the test
Jo Liss: but do we have a better use case than xml?
Scott González: jsonlint?
Scott González: We actually use XML for api.jquery_.com
Jo Liss: *nods_

@wycats
Copy link

wycats commented Feb 20, 2014

The approach we're planning on using in Ember is hierarchical logging namespaces (ember.render.template, ember.router.transition). It will also be possible to append a listener to any subset of the namespaces, which could be a simple console.log, the Log pane of the Ember Inspector, or even something that collects up errors and sends them to the server.

For example, you could imagine a listener that collects up every transition, and then sends the user's path through the app to the server together with any errors that it sees.

We're planning on having a nice logging pane in the Ember inspector that makes use of this infrastructure; it would make sense for it to be available standalone to non-Ember users as well who use any of the many libraries in the Ember ecosystem that make use of the logger.

@joliss joliss closed this as completed Feb 26, 2014
@joliss
Copy link
Member

joliss commented Mar 11, 2014

For reference, some related discussion on linting in ember-cli/ember-cli#36.

@tkellen
Copy link

tkellen commented Mar 14, 2014

@cowboy Do you feel like weighing in here?

@tkellen
Copy link

tkellen commented Mar 14, 2014

I ask because I know you've spent a TON of time thinking about this problem.

@cowboy
Copy link

cowboy commented Mar 14, 2014

I've spent a ton of time thinking about the logging problem, and it's not the easiest one to handle. All plugins of a certain type should have a standard API for logging, but I'm not 100% sure what that should be. It could be an event emitter or logging methods. The latter most likely just being a basic abstraction around the former.

Regardless of the implementation, I like these logging levels:

  • info the default logging level, basically like console.log
  • verbose somewhat chatty. used for troubleshooting the plugin configuration and maybe extra bonus logging that most people don't really need, but a few ask for. totally optional.
  • debug really chatty. used for troubleshooting the plugin source code by the plugin author
  • warn something bad happened that's not enough to fail the plugin, but the user should take note

I don't think there should be an error level, because in my mind, a plugin should fail when there's an error (therefore, there can only ever be one error) and the thing running the plugin should be responsible for reporting that.

I've been toying with the idea of a "progress" abstraction, but haven't had time to get into it lately. Either way, that could just be another thing that gets logged, maybe at the verbose level, and is probably a separate module that someone might require and pass an EventEmitter or logging method. I'm punting on this one for now.

And, of course, the thing running the plugin should listen for these events and actually log the provided messages in a sane way.

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

No branches or pull requests

7 participants