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

[Experimental] Support template co-location and Embroider #153

Merged
merged 2 commits into from
Oct 2, 2019

Conversation

dfreeman
Copy link
Member

@dfreeman dfreeman commented Oct 2, 2019

This will (in theory) address #140 and #151. The goal is for this to be a non-breaking change, and the current plan is to release this as [email protected] to give folks a chance to test and offer feedback.

At a minimum, much better test coverage will be needed before this is declared stable, but Embroider doesn't yet support template colocation, and even if it did there are still a number of other configurations we need to maintain support for.

The testing setup in this repo is already about stretched to the limit in terms of the number of variations it's intending to cover, so a prerequisite for getting this tested will be moving to a test-packages style setup with multiple test apps and addons rather than the single dummy one. Additional beta releases may follow as that gets set up and bugs or incompatibilities are discovered.

@dfreeman dfreeman merged commit 48ac3e5 into master Oct 2, 2019
@dfreeman dfreeman deleted the colocation branch October 2, 2019 18:51
@@ -24,13 +24,11 @@ module.exports = {
this.modulesPreprocessor = new ModulesPreprocessor({ owner: this });
this.outputStylesPreprocessor = new OutputStylesPreprocessor({ owner: this });
this.checker = new VersionChecker(this.project);
this.plugins = new PluginRegistry(this.parent);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is potentially a breaking change.

We have an addon like this:

module.exports = {
  name: require('./package').name,

  createCssModulesPlugin(parent) {
    return new BreakpointsPlugin(parent, {
      breakpoints: this.breakpoints
    });
  },

  included() {
    this.breakpoints = require('./breakpoints.json');
  }
}

Prior to this change, included was called before createCssModulesPlugin. Now the order is changed.

For this simple case, the fix is easy:

module.exports = {
  name: require('./package').name,

  createCssModulesPlugin(parent) {
    return new BreakpointsPlugin(parent, {
      breakpoints: this.breakpoints
    });
  },

  init(...args) {
    this._super(...args);
    this.breakpoints = require('./breakpoints.json');
  }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

☹️ My goal was for this overall release to be non-breaking, but I don't see a way around this timing change, because we now need access to the ECM options before we can set up the HTMLBars plugin. Technically we never made any promises about invocation order, but hiding behind that rules-lawyering feels pretty bad.

On the public registry, at least, I think you and I are the only two authors of packages with the ember-css-modules-plugin keyword, though of course there's no way to know about private packages. Do you have any sense of how much this is likely to impact the plugins you've written? I don't think any of mine (internal or public) are relying on it, but they're mostly fairly simple.

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally and speaking for @ClarkSource I'm totally fine with this change. None of my public plugins break, and of all private ones this was the only one. 🎉

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 Ok, we'll see if anyone else runs into this during the beta, and assuming they don't we'll plan to keep this in as a non-major change under the banner of "undefined behavior can't break"

```

```css
/* app/components/my-component.css */

Choose a reason for hiding this comment

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

is app/components/my-component/index.css also supported?

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.

3 participants