Embroider compatibility

dfreeman commented 5 years ago

I haven't spent a lot of time thinking through this deeply yet, but the mental model for asset inclusion with Embroider is fairly different from the v1 version of the addon ecosystem.

This ultimately may or may not result in a change to the way users of this addon interact with CSS modules, but the guts of ECM today are almost certainly incompatible with Embroider and will need to be restructured for the new system.

kiwiupover commented 5 years ago

@dfreeman thanks for your work on ECM. Do you have any idea what it would take to make ECM compatible with Embroider?

lennyburdette commented 5 years ago

I decided to check this out since ember-css-modules was the first obvious breakage when we tried Embroider.

In an app, I can get a basic usage working with two hacks:

Generated JS modules are getting the app prefix twice: they're registered as host-app/host-app/styles/components/foo. I can fix this by removing the "ownerName" from the dist path in the modules preprocessor.
The options.meta.moduleName in the the htmlbars plugin is an absolute path, like /private/var/folders/d6/rsf2l61x0x59gy2hsk23y04h0001sx/T/embroider/3e0a63/host-app/components/foo.js. Stripping off Embroider's temp directory here corrects the from option passed to {{local-class}}.

I have no idea what the fixes are, of course! But hopefully this is helpful.

I believe there are additional issues when used in an addon—I'll take a look at report back.

lennyburdette commented 5 years ago

So the issues are similar with addons (at least an addon in a yarn workspace.) The options.meta.moduleName is still incorrect, but it's a bit more complicated:

For starters: embroider is running the htmlbars-plugins twice ... sorta. It always runs once with a moduleName includes the Embroider temp directory in it.

It also runs with a moduleName a relative path like templates/components/bar.hbs without the addon's namespace, but only if the template content is invalidated (this was fun to figure out!)

The latter issue blows up early because this function returns undefined. Once that's fixed, the latter run of the htmlbars-plugin seems to win and the local-class helper fails at runtime (because it looks for the js module without the addon's namespace.)

I think this is good news though! Tweaking a funnel output path and fixing the meta.moduleName should be sufficient to make this compatible with Embroider.

@ef4 would love to hear your thoughts!

ef4 commented 5 years ago

@lennyburdette if you can share a reproduction of the same template getting processed both ways (with and without an absolute path in moduleName, that would be a helpful bug report. My expectation is that what you're seeing is that addon code gets preprocessed differently than app code. See also https://github.com/embroider-build/embroider/issues/141

Overall, the next steps I see here are:

I think small changes to either this addon or embroider will be all that's required to get backward compatibility. That's enough to let people who use css-modules use embroider.
Then we should get an "embroider-native" reimplementation working. This would involve directly configuring things like css-loader. My expectation is that this will be a huge simplification over how this addon is forced to work today. And it will result in better optimization. For example, the styles for a component will shake out when the component does, and load lazily if the component does.
Finally, before we can call it all stable and done, we need to codify exactly the kind of things an addon like ember-css-modules wants to configure in embroider, and actually make our own public API for those things. Step 2 was about experimentation to show what's needed, but we really don't want arbitrary addons trying to compose webpack configs, because that

i. locks us into webpack (and probably even a particular webpack version), when we want to be free to adopt future general-purpose packaging tools that don't even exist yet, and

ii. gives us poor control over how configuration composes.

dfreeman commented 5 years ago

I probably won't be able to dig deep on this in the next few days, but I'm hoping to have some time for it next week.

Off the top of my head, it wouldn't surprise me if the double processing for addon files is our fault on the ECM side. We're plucking and merging some trees around to try and get a consistent view of the universe, and that looks a bit different for apps vs addons (but it's pretty fragile for both).

@ef4 is there a sanctioned way for a v1 addon to detect that it's being consumed via Embroider's compatibility layer? For step 1, it seems like that might provide the simplest path to compatibility without rocking the boat for existing consumers.

buschtoens commented 5 years ago

Some of these problems sound reminiscent of what I had to do in #128 (not merged) for achieving Module Unification compatibility.

lennyburdette commented 5 years ago

Here's a repo that reproduces the issues, with some hacks to get things temporarily working: https://github.com/lennyburdette/ember-css-modules-embroider

I dug in a bit more and discovered that ember-css-modules's HTMLBars plugin for addon templates is instantiated by embroider twice:

First in applyTransforms. The moduleName here is missing the addon namespace, and this runs only when the template content is invalidated.
Second in precompile. The moduleName here has the embroider temp directory in it, and this runs every time I start the server.

josemarluedke commented 5 years ago

@dfreeman Any plans to continue pursuing making css-moduels work with Embroider? Looking forward to having this working, which would put us closer to be able to use Embroider.

dfreeman commented 5 years ago

@josemarluedke Yes! Unfortunately I've been pretty deep in a couple other projects at work recently and haven't had the chance to come back to this, but some necessary changes landed in Embroider not too long ago. My hope is to come back and get a drop-in Embroider compatible release out the door sometime in the next few weeks to unblock folks who are having to wait to adopt because of ECM.

dfreeman commented 5 years ago

I wanted to post an update on this, since I know a lot of folks are anxious to eliminate ECM as an Embroider adoption hurdle. I have a branch locally that works for apps and addons on an initial build with Embroider, but at present Embroider isn't picking up CSS changes on rebuild—though interestingly, the JS modules do update.

Tl;dr: this hasn't been forgotten, and forward progress is still happening! I'll continue to update here periodically, though hopefully before too much longer the update will just be "it's supported" and that will be that 🙂

kiwiupover commented 5 years ago

Thanks heaps for the update.

On Thu, Aug 29, 2019 at 6:33 AM Dan Freeman notifications@github.com wrote:

I wanted to post an update on this, since I know a lot of folks are anxious to eliminate ECM as an Embroider adoption hurdle. I have a branch locally that works for apps and addons on an initial build with Embroider, but at present Embroider isn't picking up CSS changes on rebuild—though interestingly, the JS modules do update.

Tl;dr: this hasn't been forgotten, and forward progress is still happening! I'll continue to update here periodically, though hopefully before too much longer the update will just be "it's supported" and that will be that 🙂

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/salsify/ember-css-modules/issues/140?email_source=notifications&email_token=AAE6EC3RSH7RM5TNWUYI343QG7F2DA5CNFSM4HDSVEBKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5OPTXI#issuecomment-526186973, or mute the thread https://github.com/notifications/unsubscribe-auth/AAE6ECZ5QK2DHHPPL5IHZSLQG7F2DANCNFSM4HDSVEBA .

josemarluedke commented 5 years ago

Thanks for the update @dfreeman. Just FYI, for styles not updating, that's not a particular bug in ECM. It is a bug in Embroider and it is happening in regular CSS files. See https://github.com/embroider-build/embroider/issues/309.

dfreeman commented 5 years ago

@josemarluedke 🤦‍♂ I was about 2 minutes away from coming to that conclusion myself, I think—I can see that the file in <workspace>/node_modules/@embroider/synthesized-styles/assets/<app-name>.css is being updated, but the one in <workspace>/assets/<app-name>.css isn't.

I was reaching the "how on earth is this working when you take ECM out of the equation" point, and was about to go test that out.

NullVoxPopuli commented 5 years ago

@dfreeman I can test that branch via ember-cli-notifications in emberclear, if ya need -- I'm hoping that it's my last blocker for embroider -- and then pushing it to production :)

NullVoxPopuli commented 5 years ago

My specific issue is that that embroider tmp directory for ember-cli-notifications doesn't include the styles directory.

that addon imports styles via: https://github.com/ynnoj/ember-cli-notifications/blob/master/addon/components/notification-container.js#L5

and my build gives this error:

Build Error (PackagerRunner)

Module not found: Error: Can't resolve '../styles/components/notification-container' in '/tmp/embroider/25570d/node_modules/ember-cli-notifications/components'

dfreeman commented 4 years ago

There's now experimental support for Embroider in 1.3.0-beta.1. Note that rebuilds won't update your concatenated app-name.css file until https://github.com/embroider-build/embroider/issues/309 is fixed, but if you have the opportunity to give the beta a try and report back with what works or doesn't that would be great!

In the coming days/weeks we'll be fleshing out multiple test apps/addons with different build configurations here, so the more input we have to make sure we're covering our bases, the better 🙂