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

Embroider compatibility #140

Open
dfreeman opened this issue Apr 4, 2019 · 27 comments
Open

Embroider compatibility #140

dfreeman opened this issue Apr 4, 2019 · 27 comments

Comments

@dfreeman
Copy link
Member

dfreeman commented Apr 4, 2019

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
Copy link

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

@lennyburdette
Copy link

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
Copy link

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
Copy link
Contributor

ef4 commented Apr 24, 2019

@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 embroider-build/embroider#141

Overall, the next steps I see here are:

  1. 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.

  2. 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.

  3. 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
Copy link
Member Author

dfreeman commented Apr 24, 2019

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
Copy link
Contributor

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

@lennyburdette
Copy link

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
Copy link

@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
Copy link
Member Author

@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
Copy link
Member Author

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
Copy link

kiwiupover commented Aug 29, 2019 via email

@josemarluedke
Copy link

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 embroider-build/embroider#309.

@dfreeman
Copy link
Member Author

@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
Copy link

NullVoxPopuli commented Aug 30, 2019

@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
Copy link

NullVoxPopuli commented Aug 30, 2019

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
Copy link
Member Author

dfreeman commented Oct 2, 2019

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 embroider-build/embroider#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 🙂

tansongyang added a commit to dustinsoftware/octane-pwa that referenced this issue Oct 24, 2019
- Need beta version of Ember CSS Modules.
See salsify/ember-css-modules#140 (comment)
- Need full path because of our settings.
See salsify/ember-css-modules@251a2d9#diff-04c6e90faac2675aa89e2176d2eec7d8R160
- Build seems to work, but when you actually visit localhost:4200, you
get a Fastboot-related error.
@danwenzel
Copy link

Awesome, @dfreeman ! Thanks a lot for your work on this.

I ran into a few issues when using the staticComponents and staticHelpers embroider options:

@dfreeman
Copy link
Member Author

dfreeman commented Nov 5, 2019

Thanks so much for taking the time to put together reproductions and open issues, @danwenzel! I suspect the staticHelpers issue will be fairly straightforward to fix; staticComponents I'm less certain about, but I'll aim to take a look this week or next and see if I can track down what's happening.

@danwenzel
Copy link

Also doesn't seem to be working with ember-css-modules-sass: dfreeman/ember-css-modules-sass#8

@danwenzel
Copy link

Hi @dfreeman - Just checking to see if you've had a chance to look into these ⬆️

@dfreeman
Copy link
Member Author

dfreeman commented Dec 9, 2019

Hi @danwenzel, sorry for the long silence here! This is near the top of my "OSS-time" list of priorities, but unfortunately I've been underwater the past month or so and just haven't had much time to put into these things. That's not likely to change over the next couple weeks, but my plan right now is to set aside some time over the holidays to burn through my backlog of pending issues like this one.

@danwenzel
Copy link

No problem, @dfreeman , thanks for the update!

@danwenzel
Copy link

Hi @dfreeman - Checking in on this one. Any updates?

@dfreeman
Copy link
Member Author

I just published a new release of ember-css-modules-sass that fixes the intermediateOutputPath issue it had in an Embroider environment, and I published 1.3.0-beta.2 here, which includes support for staticHelpers in Embroider.

Unfortunately after exploring several possible approaches for supporting staticComponents, I don't think that's something that will be easily doable operating as a v1 addon in Embroider; I left a note with a bit more detail on that on #160.

As-is, I think beta.2 is likely to be promoted to a stable release shortly. It would be great to have folks on this thread who've been using the previous beta (especially those who had issues!) kick the tires and chime in if anything seems amiss. Thanks!

@knownasilya
Copy link

What's the status on this?

@dfreeman
Copy link
Member Author

Currently we support Embroider-based builds in v1.3.0 and greater of ECM, with the caveat that the staticComponents flag remains unsupported.

@scottmessinger and I had chatted a bit about the possibility of configuring css-loader directly via @embroider/webpack to see about getting a full "Embroider-native" (or at least Webpack-native) CSS Modules build working. If that works out, it may be the short-term answer rather than ECM for folks who want full code splitting for their builds.

Longer-term I think the combination of Embroider's v2 addon api and template strict mode will likely enable a much lighter weight version of ECM that desugars to something very similar to what Scott describes here (though likely a bit more static, and without the need for a backing class).

My focus both at work and in my OSS time is largely in other areas for at least the next few months (which should hopefully give both those things a bit more time to advance), but if anyone is interested in trying to drive things forward in the meantime I'm happy to find some time to share my current thinking.

@ijlee2
Copy link

ijlee2 commented Apr 10, 2023

Based on Ember + Modern CSS, I wrote embroider-css-modules to come up with a solution that is compatible with Embroider (w/ the strictest settings), TypeScript, Glint, and <template> tag.

You can find an example of a migrated app in ember-container-query#167, but it's early to tell whether the CSS modules approach will extend well to many other Ember apps. I'm also not sure what CSS modules for addons and engines should look like just yet.

If you can try out embroider-css-modules and provide me feedback, I'd appreciate it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants