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

[WIP] Remove all addon artefacts from production build #98

Closed

Conversation

simonihmig
Copy link
Contributor

Here is something to solve #31. Still not ready to merge, needs at least some review, documentation and maybe some acceptance tests.

It introduces a new option enabled for your ember-cli-build.js, which defaults to false for a production build. So when this is false, it does the following:

  • everything from the addons app folder is removed from build, so none of the components are available
  • it does not import the other js dependencies (remarkable + highlight.js)
  • it replaces all app/styles files with an empty file. This is to not break any @import statements from the app's app.scss, but still not add any styleguide related styles to the app's CSS payload

What it does not:

This worked so far well for our app, reducing the JS payload by about 775kB! (uncompressed)

So, if this looks right for you, I can add some pieces to the docs and add some tests.

@kellyselden
Copy link

Can an addon blacklist it's own addon dependencies like an app can?

@simonihmig
Copy link
Contributor Author

@kellyselden Don't think so. The blacklist is part of EmberApp: https://github.com/ember-cli/ember-cli/blob/21a2e44619685d86327da1bb8b740e7a968f401a/lib/broccoli/ember-app.js#L425

I guess you are refering to the issue of removing the addon dependencies? shouldIncludeChildAddon(https://ember-cli.com/api/classes/Addon.html#method_shouldIncludeChildAddon) seems to be intended to cover exactly that case, but I fail to see how this is supposed to work when having to depend on the config. It would work if we would just hardcode that the styleguide is excluded from production builds (without taking the enabled flag into account), by checking process.env.EMBER_ENV in shouldIncludeChildAddon. But this would prevent the user to be able to override the defaults and include the styleguide even in a production build.

@simonihmig
Copy link
Contributor Author

@chrislopresto could I get some feedback on this?

@lukemelia
Copy link
Collaborator

@simonihmig I've been working with @chrislopresto on this topic and we are planning on going in a different direction. Instead of a custom stripping option/config, we're going to tell people how to blacklist ember-freestyle in their app's ember-cli-build.js and make sure that the standard setup works well with this. Our thinking is that this will be more maintainable than having our own code that does this.

@simonihmig
Copy link
Contributor Author

@lukemelia thanks, good to know. Afair I tried that approach as well, but wonder how you can make @import statements in app.scss not break when the addon is blacklisted?

@lukemelia
Copy link
Collaborator

@simonihmig that was an indeed a blocker. We addressed it (along with criticism from people who want to use freestyle but are not using scss in their app) here.

@chrislopresto
Copy link
Owner

@simonihmig Thanks for getting the ball rolling on all of this. Your PR was a great jumpstart for @lukemelia and me the other day.

As we reviewed this PR (and related approaches), we realized that stripping ember-freestyle bits in this fashion still left us with problems in scenarios where an app included an addon that itself included ember-freestyle.

We made a few simplifying decisions to get the ball rolling on a few fronts:

  • Swap out ember-remarkable for ember-cli-showdown to remove the need for bower dependencies and an indirect dependency on highlight.js
  • Load highlight.js from CDN to avoid bloating the ember-freestyle vendor.js payload
  • Move CSS to the addon tree to avoid taking an opinion on consuming application CSS pre- and post-processors... and to simplify the process of blacklisting / stripping ember-freestyle from production builds

Thanks again for the PR, the ideas, the payload reduction stats, etc. It really underscored the need to figure this out ASAP.

I'm going to close this PR and a few others now that we have carved out a direction. Feel free to reach out with thoughts and questions as we work to get the next release out the door.

Cheers!

@simonihmig
Copy link
Contributor Author

Ok guys, appreciate the feedback, and looking forward to your solution and the next release!

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.

4 participants