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

Re-architect plugin loading and asset detection logic (add Embroider support) #327

Merged
merged 13 commits into from
Oct 26, 2021

Conversation

thoov
Copy link
Contributor

@thoov thoov commented Oct 12, 2021

For apps (ember-cli-build.js):

let app = new EmberApp(defaults, {
  babel: {
    plugins: [...require('ember-cli-code-coverage').buildBabelPlugin()],
  },
});

For embroider apps (ember-cli-build.js):

let app = new EmberApp(defaults, {
  babel: {
    plugins: [...require('ember-cli-code-coverage').buildBabelPlugin({ embroider: true })],
  },
});

For in-repo addons (index.js):

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

  options: {
    babel: {
      plugins: [...require('ember-cli-code-coverage').buildBabelPlugin()],
    },
  },
};

For in-repo engines (index.js):

module.exports = EngineAddon.extend({
  // ...
  included() {
    this._super.included.apply(this, arguments);
    this.options.babel.plugins.push(...require('ember-cli-code-coverage').buildBabelPlugin());
  },
});

tests/index.html:

<script src="/coverage.js" integrity="" data-embroider-ignore></script>

tests/test-helpers.js:

import { forceModulesToBeLoaded } from 'ember-cli-code-coverage/test-support';

forceModulesToBeLoaded();

@thoov
Copy link
Contributor Author

thoov commented Oct 12, 2021

cc: @rwjblue

@thoov thoov force-pushed the embroider-support branch from 35e3210 to 6ad2450 Compare October 13, 2021 23:21
Copy link
Collaborator

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

This is great!!! I left some comments inline, and I also think the README needs some updates; but overall I love it!

packages/ember-cli-code-coverage/index.js Outdated Show resolved Hide resolved
packages/ember-cli-code-coverage/index.js Show resolved Hide resolved
packages/ember-cli-code-coverage/index.js Outdated Show resolved Hide resolved
packages/ember-cli-code-coverage/index.js Show resolved Hide resolved
@rwjblue rwjblue mentioned this pull request Oct 18, 2021
3 tasks
@thoov thoov force-pushed the embroider-support branch from 6ad2450 to 1c50bfa Compare October 21, 2021 21:19
@thoov thoov changed the title DO NOT MERGE: Adds Embroider support + restructure Adds Embroider support + restructure plugin loading and asset detection logic Oct 21, 2021
@thoov thoov changed the title Adds Embroider support + restructure plugin loading and asset detection logic Add Embroider support, rearchitect plugin loading and asset detection logic Oct 21, 2021
thoov added 2 commits October 21, 2021 14:40
 - Adding support for Embroider
 - Restructure loading of babel plugin
 - Restructure asset logic
@thoov thoov force-pushed the embroider-support branch from 1c50bfa to 621a7e7 Compare October 21, 2021 21:40
@rwjblue rwjblue changed the title Add Embroider support, rearchitect plugin loading and asset detection logic Re-architect plugin loading and asset detection logic (add Embroider support) Oct 25, 2021
Copy link
Collaborator

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Some small inline comments, but overall this is looking really great!

I think other than the inline comments to address, we need to tweak the README on how to consume (I think your PR description code snippets are good enough there though).

packages/ember-cli-code-coverage/index.js Outdated Show resolved Hide resolved
packages/ember-cli-code-coverage/index.js Outdated Show resolved Hide resolved
if (opts.embroider === true) {
let {
stableWorkspaceDir,
} = require('@embroider/compat/src/default-pipeline');
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to get this re-exported from the top level of @embroider/compat. Maybe we can follow up with that?

packages/ember-cli-code-coverage/lib/attach-middleware.js Outdated Show resolved Hide resolved
packages/ember-cli-code-coverage/index.js Outdated Show resolved Hide resolved
packages/ember-cli-code-coverage/package.json Show resolved Hide resolved
@thoov
Copy link
Contributor Author

thoov commented Oct 26, 2021

cc: @rwjblue Can you take another look at this

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Co-authored-by: Katie Gengler <katie@kmg.io>
@rwjblue rwjblue merged commit d2d387d into ember-cli-code-coverage:master Oct 26, 2021
@villander
Copy link

@thoov @rwjblue and how about the standalone engines here?

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

Successfully merging this pull request may close these issues.

4 participants