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

Add app in top level config #7

Merged
merged 8 commits into from
Mar 19, 2019
Merged

Conversation

steventsao
Copy link
Contributor

  • updated readme
  • updated tests to handle the new config

@steventsao
Copy link
Contributor Author

steventsao commented Mar 14, 2019

Resolves #2.

- updated readme
- updated tests to handle the new config
Copy link
Member

@simonihmig simonihmig left a comment

Choose a reason for hiding this comment

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

Thanks @steventsao, this looks very good!

This is a breaking change, as any old (flat) config would not work with the new approach, right?
I guess you could provide an upgrade path, by supporting both formats and deprecating the previous one. But tbh, I am not that much worried about it, we can certainly just bump the minor version and tell users how to upgrade...

What I am really surprised about is that the blueprint test is not failing! It seems you (rightly) changed the default blueprint, but forgot to update the test fixture (https://github.com/kaliber5/ember-cli-bundlesize/blob/master/node-tests/fixtures/config/bundlesize.js). No big deal, but I really don't understand why this was not failing the test!? 🤔

@steventsao
Copy link
Contributor Author

steventsao commented Mar 19, 2019

What I am really surprised about is that the blueprint test is not failing! It seems you (rightly) changed the default blueprint, but forgot to update the test fixture (https://github.com/kaliber5/ember-cli-bundlesize/blob/master/node-tests/fixtures/config/bundlesize.js). No big deal, but I really don't understand why this was not failing the test!? 🤔

It looks like the test only passed because emberGenerate was used incorrectly and the assertion had timed out. The function returns a promise and does not take a callback that returns a file. I've moved the assertion to after the promise resolves and to compare their buffers instead. LMK what you think!

@simonihmig
Copy link
Member

It looks like the test only passed because emberGenerate was used incorrectly and the assertion had timed out.

Seems you are right, thanks for investigating!

I've moved the assertion to after the promise resolves and to compare their buffers instead.

I prefer not to compare buffers, but still use the file helper for that. As in the examples. What's missing in the examples is the part to import the file helper, which can be done like here. Would you be able to update that accordingly?

@steventsao
Copy link
Contributor Author

steventsao commented Mar 19, 2019

Done. the error case looks much better than using buffer.

AssertionError: expected "config/bundlesize.js" to equal "/.../../fixtures/config/bundlesize.js"
      + expected - actual

       'use strict';

       module.exports = {
      -  app: {
      +  appppppp: {

Copy link
Member

@simonihmig simonihmig left a comment

Choose a reason for hiding this comment

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

Looks awesome, thank you!

@simonihmig simonihmig merged commit 7e3dd1d into kaliber5:master Mar 19, 2019
@simonihmig
Copy link
Member

Published as 0.1.0! 🎉

@steventsao steventsao deleted the develop branch March 19, 2019 16:53
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.

2 participants