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

Bundle CSS files #205

Merged
merged 2 commits into from
Apr 18, 2019
Merged

Bundle CSS files #205

merged 2 commits into from
Apr 18, 2019

Conversation

bendemboski
Copy link
Contributor

I took a stab at the most basic level of support for CSS. These changes make ember-auto-import bundle any CSS found in the webpack output, so by specifying a webpack config that collects CSS, e.g. mini-css-extract-plugin, we can include CSS in Ember's vendor.css.

My personal use case is integrating CKEditor5 into my build. Rather than using one of their pre-built packages, I want to build/customize my own, and consume their source packages using ember-auto-import to mimic the webpack build they use to generate the pre-built packages. It appears to work seamlessly for the Javascript, but I need to do something like this to get the CSS, which motivated this PR.

A few notes about this PR:

  1. I've not spent much time in ember-auto-import's code, so please let me know if this isn't the right approach.
  2. I wonder if bundling CSS should be opt-in via a config? I like opt-in, but I think the only way to get CSS into ember-auto-import's tree is by customizing the webpack config to do so, so maybe that's opt-in enough?
  3. In the unit tests, I wasn't quite sure how many tests should include CSS files -- all of them seemed overkill since CSS and JS share the same code path (just with different values). So please let me know if you think I added CSS testing to too many or too few tests!

If the webpack config causes css files to be included in the output, they will be appended to vendor.css/test-support.css.
@ef4
Copy link
Collaborator

ef4 commented Apr 8, 2019

Thanks, this looks nice. I'm fine with building in CSS bundling, as long as we codify what semantics we're supporting (rather than locking into "whatever mini-css-extract-plugin does".

I was already planning to go this same direction with embroider, and ember-auto-import is (in one sense) a polyfill for embroider. The embroider addon spec has this section on CSS.

Could you add the appropriate webpack config to this PR, so it works out-of-the-box?

@ef4
Copy link
Collaborator

ef4 commented Apr 9, 2019

(The webpack config gets generated here, and if you're adding a plugin we should be sure to require.resolve it first so it's looked up relative to ember-auto-import, rather than relative to the app.)

@bendemboski
Copy link
Contributor Author

@ef4 as a disclaimer, I'm kind of a webpack n00b -- I've barely used it outside the purview of ember-auto-import. I guess it's possible that I stumbled on the de facto standard for collecting CSS using webpack, but I kinda figured that this was just one of a bunch of different methods of doing so, so I was thinking that ember-auto-import would stay agnostic as to how to configure webpack to collect/generate CSS, and would just package up the CSS it finds, which was the aim of this PR.

I also had some vague notions about maybe in the future providing a config to ember-auto-import that tells it how to bundle up the webpack output into CSS files in case people want to put it into different CSS files, e.g. if the library they are including creates content in an iframe then the developer could bundle its CSS into a separate CSS file from vendor.css that is only loaded into the iframe. But I kinda pictured this as a mapping from webpack-generated files to output paths where the developer would just have to coordinate the webpack config and the mapping so the one created correct file paths to feed into the other.

Anyway, if somebody more experienced with webpack than I am says that tightly coupling ember-auto-import to some mini-css-extract-plugin webpack configuration will cover pretty much everybody's use cases, I'm happy to go that route, but I certainly don't feel qualified to make that call, which is why I left this webpack-config-agnostic. In fact, I'm not quite sure how a webpack config baked in to ember-auto-import would meet my particular use case, since the CKEditor5 webpack config includes some customized/generated config options.

So please let me know how you'd like to proceed here -- I'm game for anything, I just want to be clear that I have not nearly enough webpack domain knowledge here to make the call myself.

@bendemboski
Copy link
Contributor Author

Hey @ef4 any more thoughts here? I'm getting to the point in my project that I'm going to have to run off a local tarball of ember-auto-import or something, so I'd love to see this merged and released sometime soon. If we have more figuring to do, then I'll manage, but want to give you a poke to see what we can do.

Copy link
Collaborator

@ef4 ef4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I now I understand better what you're trying to achieve here. We're really just making it so that if somebody configures webpack to emit CSS, we will grab it and include it into the ember app. I'm fine with that as a lower-level feature for the present.

I noticed one small type issue, if you can fix that up we can merge this.

Thanks for all this effort.

@@ -200,7 +209,7 @@ interface PassthroughEntry extends WalkSyncEntry {

type AugmentedWalkSyncEntry = WalkSyncEntry | PassthroughEntry;

function findByPrefix(path: string, map: Map<string, string>) {
function findByPrefix(path: string, map: Map<string, any>) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can replace any here with Map<string, string>?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can't just be Map<string, string> because we call it with both this.passthrough and this.mappings, and this PR changes this.mappings to Map<string, Map<string, string>> (but leaves this.passthrough as Map<string, string>). So maybe we could do Map<string, string | Map<string, string>> but I thought since the function itself is agnostic as to what the values of the map are, it made sense to leave it as any. But I'm happy to change it to an | if you prefer.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, in that case it should be:

function findByPrefix<T>(path: string, map: Map<string, T>) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, TIL! I'm still kinda new to typescript also. I just pushed a commit (didn't squash/rebase or anything -- figured you could just do that when merging).

@ef4 ef4 merged commit ec75dae into embroider-build:master Apr 18, 2019
@ef4
Copy link
Collaborator

ef4 commented Apr 18, 2019

Released in 1.3.0.

@bendemboski
Copy link
Contributor Author

🎉 thank you!

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

Successfully merging this pull request may close these issues.

2 participants