[WIP] Ember cli classic #185
Conversation
- Make work with dummy apps. - Delivers runtime helpers (currently in "tmp.js" file, Ember isn't resolving the helpers correctly...)
- Each working tree has its own Analyzer and Transport now. - Transport objects are now backed by a Class with a reset method. - Re-organized addon to use functional getEnv and getOptions methods.
|
|
||
| // If we're in Classic or Pods mode, every hbs file is an entry point. | ||
| if (discover && path.extname(file) === ".hbs") { this.entry.push(file); } | ||
|
|
||
| await fs.ensureDir(path.join(this.outputPath, path.dirname(file))); |
There was a problem hiding this comment.
let transition to sync FS functions.
- in general they are more performant (no need to schedule to the event loop and come back), and as we are processing serially through a build. Rarely (if every) does the non-blocking aspect help.
- they make lots of sense for a webserver, as that way we don't accidentally block 1 request, do to a second request doing sync IO on a slow FS.
- typically sync errors have better stack traces (although maybe newer node's fixed this)
There was a problem hiding this comment.
Doesn't this foreclose on the ability to run multiple Broccoli plugins concurrently in the future?
There was a problem hiding this comment.
I don't believe so. We're already running multiple instances of this broccoli plugin (one for each addon, engine, or app). Broccoli (according to spenner) runs the build step synchronously already, so we don't actually gain any perf benefits from async functions and instead are just left with that extra throwaway promise being created for each file system call.
If Broccoli begins running certain build steps asynchronously, this may be a perf bottleneck, but we'll need to update synchronous fs calls across the entire ecosystem to un-block it.
From a CSS Blocks perspective, there is no difference in sync vs async.
| @@ -0,0 +1,124 @@ | |||
|
|
|||
| const e = (m) => { throw new Error(m); }; | |||
There was a problem hiding this comment.
this module looks pretty complex, do we have associated unit tests?
There was a problem hiding this comment.
Copy pasta from the unit tested version in @css-blocks/glimmer because Ember isn't resolving it at build time – issue is commented in the helpers file. This will have to be resolved before it can be merged.
| let componentDeps = this.resolver.recursiveDependenciesForTemplate(componentName); | ||
| componentDeps.forEach(c => components.add(c)); | ||
| } catch(e){ | ||
| this.debug(`Warning: Could not discover recursive dependencies for component ${componentName}`); |
There was a problem hiding this comment.
If we do intend to catch here, we should likely only catch errors we expect. For example, we should likely not catch TypeErrors and similar.
That way, we do not hide real issues.
QQ: do we intend to catch here going forward?
There was a problem hiding this comment.
This catch can likely be removed since I've refactored a bit more, will look into it.
| }); | ||
|
|
||
| this.debug(`Analyzing all components: ${[...components].join(", ")}`); | ||
|
|
||
| components.forEach(dep => { | ||
| analysisPromises.push(this.analyzeTemplate(dep, depAnalyzer)); | ||
| analysisPromises.push(this.analyzeTemplate(dep)); |
There was a problem hiding this comment.
Map with side effects makes me squirm...but okay. Will look like:
[...components]
.map(this.analyzeTemplate, this)
.map(Array.prototype.push, analysisPromises);There was a problem hiding this comment.
Map with side effects makes me squirm
Seems ok then.
| // First try Classic Ember structure. | ||
| let classic = template.replace('templates/', 'styles/').replace('.hbs', '.block.css'); | ||
| classic = path.join(this.projectDir, this.srcDir, classic); | ||
| if (await fs.pathExists(classic)) { |
| let pods = path.parse(template); | ||
| pods.base = "stylesheet.block.css"; | ||
| let podsPath = path.join(this.projectDir, this.srcDir, path.format(pods)); | ||
| if (await fs.pathExists(podsPath)) { |
| let stylesheet = await this.tmplPathToStylesheetPath(identifier); | ||
| if (!stylesheet ) { return undefined; } | ||
| return new ResolvedFile( | ||
| (await fs.readFile(stylesheet)).toString(), |
There was a problem hiding this comment.
readFileSync(stylesheet, 'UTF8')
| let file: string = this.depAnalyzer.project.resolver.resolve(identifier); | ||
| file = path.join(this.projectDir, this.srcDir, file); | ||
|
|
||
| let content = (await fs.readFile(file)).toString(); |
| if (!this.depAnalyzer) { | ||
| let template = path.join(this.projectDir, this.srcDir, identifier); | ||
| return new ResolvedFile( | ||
| (await fs.readFile(template)).toString(), |
| let template = this.depAnalyzer.project.templateFor(identifier); | ||
| return new ResolvedFile(template.string, template.specifier, template.path); | ||
| } catch(e) { | ||
| return undefined; |
There was a problem hiding this comment.
We should likely only catch errors we expect, re-throwing exceptional errors. This prevents hiding bugs.
- Enable Broccoli build integration to control Analyzer's root directory. - Remove the need for srcDir and rootDir discovery in ember-cli addon by using brococli tmp dirs. - Use sync fs methods.
|
Addon Status Update: The addon is now a good broccoli citizen and does not reach outside of its tree at at build time 🎉 I'm now focusing on making existing tests pass again and getting addon test coverage. Part of that will be to verify that deeply nested addons don't break the build. Once this is finished, I think the PR can be pulled in to master a partial ember-cli addon implementation! More iterative work must then be done to:
I'll be writing RFCs for these changes after the addon lands in master. |
Allows Broccoli to pass working directory at runtime without creating the analyzer instance.
- Glimmer helpers re-named to sensable private name. - Helper runtimes are injected by ember-cli addon. - Abstracted helper names out of runtime tests for maintainability. - Full test suite passes again and @css-blocks/ember-cli tests helper delivery.
- Added test case routes to dummy app with Acceptance tests. - Added BEM_UNIQUE output mode where BEM classes are guarenteed unique within the same process. - Improved output css-blocks aggregator and cleaned up some of the shared memory constructs. - Fixed css output duplication bugs. - Fixed bugs with autoreload. - Made addon work with in-repo dummy addons.
| await visit("/route-block"); | ||
| assert.equal(currentURL(), "/route-block", "Navigated to test case"); | ||
| assert.ok(varIsPresent("#scope", "route-block-scope"), "Scope style applied to root element"); | ||
| assert.ok(varIsPresent("#sub-class", "route-block-class"), "Sub-classes applied to children"); |
There was a problem hiding this comment.
I think it can follow in a separate PR, but I think we need to design some test helpers for ember unit tests that allow the developer to make assertions about block styles. Something like:
import { testHelpers as blockAssertions } from "@css-blocks/ember-cli";
async function (assert) {
let block = blockAssertions("path/to/file1.block.css");
let blockScopeElement = document.getElementById("block1");
let blockClassElement = document.getElementById("block1child");
block.assertCurrentStyle(blockScopeElement, ":scope");
block.assertCurrentStyle(blockClassElement, ".aClass[state|foo][state|bar]");
}There was a problem hiding this comment.
Yeah I was thinking the same thing 👍
- Eager engines work and have tests. - Skipped tests for lazy engines. - Bumped ember-cli and associated packages to 3.3.0. - Improve transport object and analysis lookup system.
|
@stefanpenner, @chriseppstein, I think this is ready for another review! At your leisure 🙂 For newcomers just discovering this PR, here are the caveats for this proof-of-concept ember-cli plugin:
All six of the above items will need to be finished in order to call this addon "done". I'll be tracking these all in separate tickets. But otherwise, this works for unlocking CSS Blocks syntax in your addons and apps 🎉 |
- fix: Auto-discover common preprocessor extensions for output file. - test: Add failing test page for node modules resolution use case.
This reverts commit 85d9707.
- Make ember-cli work with classic ember apps (non-module-unification). - Still works for non-Ember Glimmer apps. - Added BEM_UNIQUE output mode where BEM classes are guaranteed unique within the same process. - Analyzer.analyze method accepts working directory instead of constructor. Allows Broccoli to pass working directory at runtime without creating the analyzer instance. - Abstract stylesheet resolver from Broccoli plugin to support both Glimmer and Ember with minimal code forks. Depend on experimental fork of glimmer-analyzer for now. - Transport objects are now backed by a proper Class interface instead of a POJO. - Abstract the addon's env configuration with getEnv and getOptions methods. - Use sync fs methods in broccoli and ember-cli for better build perf. - Glimmer helpers re-named to sensible private values. - Build @css-blocks/glimmer to CJS *and* AMD so Ember can import the runtime. - Helper runtimes are automagically injected by ember-cli addon. - Abstract helper names out of runtime tests for maintainability. - Ember-CLI Template Discovery Test suite for addons, engines (eager and lazy), and apps (real and dummy). Lazy engines and some app features are failing and skipped. - Bump ember-cli and associated packages to 3.3.0. - Have Travis test with node versions 8 and 10. - Add global disable option. - Auto-discover common preprocessor extensions for Ember's `app.css` output file. - Add failing test case for node modules resolution use case.
|
does this enable support for ember's module unification? |
No description provided.