-
Notifications
You must be signed in to change notification settings - Fork 152
Add broccoli test #90
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
Conversation
chrisrng
commented
Apr 13, 2018
- Promote Analyzer object from interface to class - MetaAnalysis => Analyzer - Added basic broccoli plugin - /Block => /BlockTree - /TemplateAnalysis => /Analyzer
- JSX analyzer extends from css-blocks Analyzer and uses new APIs for getting Analyses. - Analysis objects propagate Style trackings up to parent Analyzer if present. - Analysis and TemplateInfo objects now take the template type generic string.
- Added comments and jsdoc annotations. - Template Analyzers use `startElement` and `endElement` APIs. - Rewriters' element analyzers now use special syntax, easily refactored later when we address #84. - Old `addElement` method now gone from base Analysys object. - Annotate more broccoli-css-blocks future features in comments.
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.
Largely LGTM after my couple comments are addressed 👍
Looks like the git history may still be wonky against master though. Is the plan to land this in master asap, or continue iterating?
entry: string[]; | ||
output: string; | ||
// tslint:disable-next-line:prefer-whatever-to-any | ||
analyzer: Analyzer<any>; |
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.
Why not use typeof TemplateTypes
here?
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.
Had a type error in let styleMapping = new StyleMapping
but resolved that so we're using TemplateTypes
class BroccoliCSSBlocks extends BroccoliPlugin { | ||
|
||
// tslint:disable-next-line:prefer-whatever-to-any | ||
private analyzer: Analyzer<any>; |
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.
And here
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.
Same resovled
let compiler = new BroccoliCSSBlocks(input.path(), { | ||
entry: [entryComponentName], | ||
output: "src/ui/styles/css-blocks.css", | ||
transport: {}, |
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.
Lets add a test verifying that the plugin populates our transport object with expected keys. Eventually this will be a standard object delivered by CSS Blocks core, but for now we should verify that it at least adds the expected keys.
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.
Added a test 👍
@amiller-gh Looking to land to master – we can squish it I guess. This also contains the work you have done in the PR #87 :) I've added the test for transport object |
let optimizer = new Optimizer(this.optimizationOptions, this.analyzer.optimizationOptions); | ||
|
||
// This build step is *mostly* just a pass-through of all files! | ||
// QUESTION: Tom, is there a better way to do this in Broccoli? |
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.
Don't block on this, but has this question been answered? 🙂
let filename = filesystemPath || options.importer.debugIdentifier(block.identifier, options); | ||
|
||
// If this Block has a representation on disk, remove it from our output tree. | ||
// TODO: This isn't working right now because `importer.filesystemPath` doesn't return the expected path... |
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.
Note to myself: file a ticket for this.
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.
LGTM 👍