Skip to content

[WIP] feat: Augment @css-blocks/ember-cli to be a fully-fledged ember addon. #174

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

Closed
wants to merge 2 commits into from

Conversation

amiller-gh
Copy link
Contributor

@amiller-gh amiller-gh commented May 17, 2018

Major Work in Progress!

Depend on small changes to glimmer-analyzer and @glimmer/resolution-map-builder

Currently:

  • Works in Ember apps using Module Unification.
  • Dummy app works with CSS Blocks.
  • Modify glimmer analyzer to use more concepts from Glimmer's template tree analysis package.
  • Broccoli plugin cleans up blocks files correctly now.
  • Glimmer playground demo uses updated ember-cli addon.

Still To Do:

  • Integration tests for Ember dummy app.
  • Integration tests in "playground" Glimmer app.
  • Interfaces around Analyzer, Analysis and template -> analysis lookup can be improved.
  • Provide support for referencing module identifiers in @block-references. This will require re-integration with the custom loader already in-repo.
  • Improve options validation at all layers.

Open questions:

  • We need to discuss the story around module maps, how they are constructed, how and when addons may augment them, and what access build tooling has to it. Currently module maps are hard-coded. Ideally we're using a sanctioned API for this.
  • In a similar vein, it is likely possible to improve discovery of application routes. How can we do this better?
  • How do we best support engines?
  • Do we deliver a standalone css-blocks.css css file and inject a new <link> tag in the base page, or do we automatically concat our styles with app.css or some other file?
  • A number of the code splits around Glimmer/Ember can probably be resolved with small changes to the vanilla Glimmer build pipeline that we can contribute back. Should we bother / is this wanted?

 - Works with Ember apps using Module Unification
 - Dummy app works with CSS Blocks
 - Modify glimmer analyzer to use more concepts from Glimmer's template tree analysis package.
 - Broccoli plugin cleans up blocks files correctly now.
 - Glimmer playground demo uses updated ember-cli addon.
@chriseppstein chriseppstein requested a review from tomdale May 17, 2018 17:47
@chriseppstein chriseppstein added the @css-blocks/ember-cli Issue with the CSS Blocks ember-cli integration label May 17, 2018
@ef4
Copy link

ef4 commented May 18, 2018

Glad to see this progress! I happened to also start trying to do this exact same thing this week. A few observations from comparing notes:

  • the current broccoli transform can be much simpler if it just extends an existing one like broccoli-funnel. This is also likely to make it much faster because there's a bunch of rebuild optimization stuff you would get for free.

  • I can understand why you would tie this to module unification, but I think that's really a separable concern and we can support classic apps just fine.

  • related to your question about module maps: I think that's quite separate from making css-blocks work. It's great that css-blocks can do static dependency optimization, but every existing pre-MU ember app could still get huge benefit out of css-blocks even if we treated every template as an "entrypoint" and did no module dependency crawling at all. This has led me to believe that EmberAnalyzer and GlimmerAnalyzer are probably worth doing as separate implementations.

  • that also solves the problem of how to handle route templates.

  • I don't think we should have a separate <link>, I think we should make sure css-blocks runs early enough that an existing app.css can just @import "css-blocks". That lets us interoperate with multiple different preexisting style preprocessors that people may have, which enables incremental refactorings to css-blocks.

Tomorrow I will see about getting this branch running and look for opportunities to PR some of these things.

@amiller-gh
Copy link
Contributor Author

@ef4 awesome – great feedback! Would love to see your take on this, feel free to iterate on this branch. Let me know if it gives you any trouble when spinning it up.

I will happily defer to you, @tomdale and @stefanpenner on most Ember related architecture decisions since you have far more context into that universe that Chris or I – as long as we preserve the static analysis guarantees that CSS Blocks requires 🙂

Re: module unification, I agree it would be spectacular to maintain support for classic apps, and your approach of treating each template as an entrypoint is a great way to do that. I didn't get that together because with the current implementation (module unification only) I was able to leverage much of the work already done for Glimmer.

I'd love to see apps that support module unification use the GlimmerAnalyzer and have access to all the template composition data, and if module unification is not enabled it fall back to the EmberAnalyzer – a form of CSS Blocks "graceful degradation" – is that what you were thinking?

@ef4
Copy link

ef4 commented May 18, 2018

I'd love to see apps that support module unification use the GlimmerAnalyzer and have access to all the template composition data, and if module unification is not enabled it fall back to the EmberAnalyzer – a form of CSS Blocks "graceful degradation" – is that what you were thinking?

Yes, exactly. Maybe we should even name them GlimmerAnalyzer and EmberClassicAnalyzer to make it clear that even new-style Ember apps use the GlimmerAnalyzer.

@ef4
Copy link

ef4 commented May 18, 2018

I should clarify: switching to module unification by itself is not sufficient to get a complete static dependency map of an Ember app. There are too many other dynamic escape valves, ranging from the {{component ...}} helper (which is a legit thing we want to keep, perhaps just with some additional scoping hints to make it clear which set of components are eligible to go inside of it) to things like people passing arbitrary templates to Route#render (which I would be happy to kill).

@ef4
Copy link

ef4 commented May 21, 2018

I was trying to test this branch against an actual module unification ember app and immediately ran into gaps between how MU works and how GlimmerJS works. For example, GlimmerJS doesn't understanding routes, so a hello-world ember app with just src/ui/routes/application/template.hbs blows up.

I'm going to make things work first without trying to reuse the GlimmerJS analyzer against Ember (regardless of whether using MU or not). The work to unify is not specific to css-blocks.

EDIT to add: part of what I said here was incorrect, I was confusing myself by running the wrong thing. Continuing to investigate -- jury is still out on whether we should try to use the glimmer analyzer on MU.

@amiller-gh
Copy link
Contributor Author

amiller-gh commented May 22, 2018

Hm, the addon in this branch should discover route templates / stylesheets and automatically use them as entry points – hopefully that is what your edit is referencing! If it still isn't discovering them, I'd call that a bug.

Re: MU static escape valves – I remember way back when we were building out our Glimmer experiment a while back, files using the {{component}} helper were required to add an import comment at the top of the file with all possible dynamic components used so Glimmer could properly discover them and build out the static template dependency tree:

{{!-- import ComponentName1 ComponentName2--}}
{{component dynamicName}}

If dynamicName was set to a component name that wasn't explicitly listed in the import comment at the top we would get a runtime component lookup error.

Is this still a pattern in Glimmer, and/or one that will be used in MU Ember apps?

@ef4
Copy link

ef4 commented May 22, 2018

Hm, the addon in this branch should discover route templates / stylesheets and automatically use them as entry points – hopefully that is what your edit is referencing!

Yes, I did get past that one.

Is this still a pattern in Glimmer, and/or one that will be used in MU Ember apps?

It's possible addition, but it's not part of MU as written, so it would be premature to assume anything about its exact implementation.

How have you been testing this branch? I'm unable to get the dummy app to actually run. I hit and solved the following

  • you have a fork of glimmer-analyzer and it can't run directly from github (it needs to be built first, so I'm cloning, building, and yarn linking).
  • I needed master of ember-cli with EMBER_CLI_MODULE_UNIFICATION=enabled
  • lerna bootstrap was failing to link things up properly because there is version skew between packages on this branch. I fixed that by merging master into here.

Next issue is that the analyzer is attempting to locate templates under packages/@css-blocks/ember-cli/src instead of packages/@css-blocks/ember-cli/tests/dummy/src, so the build fails. The simple fix (pointing the analyzer at ember-cli/tests/dummy instead of ember-cli) doesn't work because it also expects to find package.json. So it will need to learn about the distinction.

(But also I suspect it's incorrect to try to use glimmer-resolution-map-builder against Ember apps -- that is a glimmerJS only library. Instead the common interface is glimmer-resolver, which is tested against both glimmerJS and ember.)

@amiller-gh
Copy link
Contributor Author

amiller-gh commented May 22, 2018

How have you been testing this branch? I'm unable to get the dummy app to actually run. I hit and solved the following

I've been testing in the dummy app – your steps to fix seem correct, but make sure that you built version of glimmer-analyzer is also linked up to the forked @glimmer/resolution-map-builder I mentioned in the issue description.

Next issue is that the analyzer is attempting to locate templates under packages/@css-blocks/ember-cli/src instead of packages/@css-blocks/ember-cli/tests/dummy/src

That logic lives in @css-blocks/ember-cli/index.js:140-160. Its not particularly elegant, but the different paths are resolved there. Sounds like they're not being passed down to either glimmer-analyzer or its child @glimmer/resolution-map-builder correctly. Maybe a problem with project linking? I know I made a change to how glimmer-analyzer propagates the src directory through.

But also I suspect it's incorrect to try to use glimmer-resolution-map-builder against Ember

Probably! I'm not privy to much of the project toolchain split – will happily defer to the subject matter experts here. Although as an integrator, since both projects use resolution maps at their core, I'd expect some kind of consolidated tooling to be available ¯\_(ツ)_/¯

@ef4
Copy link

ef4 commented May 23, 2018

Incidentally, you're probably seeing annoying warnings about "Missing package directory" in simple-html-tokenizer. I fixed that upstream.

@ef4
Copy link

ef4 commented May 23, 2018

I now have this branch working locally, I was missing a few more places that needed to use the fork of @glimmer/resolution-map-builder and I had missed a recompile of some of the @css-blocks packages.

@ef4
Copy link

ef4 commented May 25, 2018

Here's a PR targeting your branch, to start adding acceptance tests. Plus it contains some questions about desired semantics: #178

@amiller-gh
Copy link
Contributor Author

Closing in favor of #185 👍

@amiller-gh amiller-gh closed this Jul 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@css-blocks/ember-cli Issue with the CSS Blocks ember-cli integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants