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

[BUGFIX] Header files import concat #6081

Merged
merged 3 commits into from Sep 27, 2016

Conversation

Projects
None yet
3 participants
@stefanpenner
Contributor

stefanpenner commented Jul 15, 2016

This ensures concats are stable, and predictably ordered. This also fixes a prepend bug for test css.

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Jul 15, 2016

Contributor

will fix patch, just a sec.

Contributor

stefanpenner commented Jul 15, 2016

will fix patch, just a sec.

@homu

This comment has been minimized.

Show comment
Hide comment
@homu

homu Sep 10, 2016

Contributor

☔️ The latest upstream changes (presumably bb4b51b) made this pull request unmergeable. Please resolve the merge conflicts.

Contributor

homu commented Sep 10, 2016

☔️ The latest upstream changes (presumably bb4b51b) made this pull request unmergeable. Please resolve the merge conflicts.

@nathanhammond nathanhammond changed the title from [Bugfix beta] Header files import concat to [BUGFIX] Header files import concat Sep 15, 2016

@stefanpenner stefanpenner changed the base branch from beta to master Sep 25, 2016

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Sep 25, 2016

Contributor

rebased + retargeted to master.

Contributor

stefanpenner commented Sep 25, 2016

rebased + retargeted to master.

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Sep 25, 2016

Contributor
  • Added tests. These tests explicitly test only ember-app's internal behavior, ensuring calls to broccoli-concat are correct, but does not test the behavior of broccoli-concat as its own test suite is responsible for that.
  • released test styles prepend: true, wasn't working and also fixed that.
Contributor

stefanpenner commented Sep 25, 2016

  • Added tests. These tests explicitly test only ember-app's internal behavior, ensuring calls to broccoli-concat are correct, but does not test the behavior of broccoli-concat as its own test suite is responsible for that.
  • released test styles prepend: true, wasn't working and also fixed that.
[BUGFIX] ensure concat order of app.import(x)
[BUGFIX] vendor test styles supports prepend: true

Files provided to app.import should always be ordered based on insertion order (either append, or prepend).
For this broccoli-concat explicitly provides `headerFiles` and `footerFiles`. `inputFiles` previously used is explicitly un-ordered.
This patch ensures the underlying libraries are correctly used, correctly enforcing expected behavior.
@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Sep 26, 2016

Contributor

windows test failure seems isolated to, which is annoyingly unhelpful (atleast from appveyors web UI)

AssertionError: expected { Object (headerFiles, outputFile, ...) } to deeply equal { Object (annotation, headerFiles, ...) }
    at Context.<anonymous> (C:\projects\ember-cli\tests\unit\broccoli\ember-app-test.js:1044:33)
    at callFn (C:\projects\ember-cli\node_modules\mocha\lib\runnable.js:334:21)
    at Test.Runnable.run (C:\projects\ember-cli\node_modules\mocha\lib\runnable.js:327:7)
    at Runner.runTest (C:\projects\ember-cli\node_modules\mocha\lib\runner.js:429:10)
    at C:\projects\ember-cli\node_modules\mocha\lib\runner.js:535:12
    at next (C:\projects\ember-cli\node_modules\mocha\lib\runner.js:349:14)
    at C:\projects\ember-cli\node_modules\mocha\lib\runner.js:359:7
    at next (C:\projects\ember-cli\node_modules\mocha\lib\runner.js:285:14)
    at C:\projects\ember-cli\node_modules\mocha\lib\runner.js:322:7
    at done (C:\projects\ember-cli\node_modules\mocha\lib\runnable.js:291:5)

45c67c4#diff-1cef190fe22e6c889bb23fda5a8296a3R1044 is the test in question (on this PR), I suspect with the full output we would have a-bit more to go on. I'll have to get a windows VM setup again and debug this

Contributor

stefanpenner commented Sep 26, 2016

windows test failure seems isolated to, which is annoyingly unhelpful (atleast from appveyors web UI)

AssertionError: expected { Object (headerFiles, outputFile, ...) } to deeply equal { Object (annotation, headerFiles, ...) }
    at Context.<anonymous> (C:\projects\ember-cli\tests\unit\broccoli\ember-app-test.js:1044:33)
    at callFn (C:\projects\ember-cli\node_modules\mocha\lib\runnable.js:334:21)
    at Test.Runnable.run (C:\projects\ember-cli\node_modules\mocha\lib\runnable.js:327:7)
    at Runner.runTest (C:\projects\ember-cli\node_modules\mocha\lib\runner.js:429:10)
    at C:\projects\ember-cli\node_modules\mocha\lib\runner.js:535:12
    at next (C:\projects\ember-cli\node_modules\mocha\lib\runner.js:349:14)
    at C:\projects\ember-cli\node_modules\mocha\lib\runner.js:359:7
    at next (C:\projects\ember-cli\node_modules\mocha\lib\runner.js:285:14)
    at C:\projects\ember-cli\node_modules\mocha\lib\runner.js:322:7
    at done (C:\projects\ember-cli\node_modules\mocha\lib\runnable.js:291:5)

45c67c4#diff-1cef190fe22e6c889bb23fda5a8296a3R1044 is the test in question (on this PR), I suspect with the full output we would have a-bit more to go on. I'll have to get a windows VM setup again and debug this

stefanpenner added some commits Sep 26, 2016

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner
Contributor

stefanpenner commented Sep 26, 2016

@ember-cli/core this should be ready for a review

cc @hjdivad


https://www.youtube.com/watch?v=hpiIWMWWVco

"vendor/ember-cli/vendor-suffix.js"
],
outputFile: "/assets/vendor.js",
separator: EOL + ';',

This comment has been minimized.

@Turbo87

Turbo87 Sep 26, 2016

Member

I'm a bit surprised by this. I thought we used \n on Windows too?

@Turbo87

Turbo87 Sep 26, 2016

Member

I'm a bit surprised by this. I thought we used \n on Windows too?

This comment has been minimized.

@stefanpenner

stefanpenner Sep 26, 2016

Contributor

@Turbo87 we actually explicitly specify EOL here. But we may want to investigate this as it doesn't seem right. As this merely tests existing behavior, should we consider the change as part of this PR or a subsequent. I can be convinced of either

@stefanpenner

stefanpenner Sep 26, 2016

Contributor

@Turbo87 we actually explicitly specify EOL here. But we may want to investigate this as it doesn't seem right. As this merely tests existing behavior, should we consider the change as part of this PR or a subsequent. I can be convinced of either

This comment has been minimized.

@Turbo87

Turbo87 Sep 26, 2016

Member

let's investigate and fix in a different PR. I was just wondering where that came from.

@Turbo87

Turbo87 Sep 26, 2016

Member

let's investigate and fix in a different PR. I was just wondering where that came from.

@stefanpenner stefanpenner merged commit 6155ddb into master Sep 27, 2016

5 checks passed

codeclimate/coverage 92.16% test coverage (+0.1%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 92.162%
Details
@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Sep 27, 2016

Contributor

landing this in canary.

Contributor

stefanpenner commented Sep 27, 2016

landing this in canary.

@stefanpenner stefanpenner deleted the header-files-import-concat branch Sep 27, 2016

@Turbo87 Turbo87 added this to the v2.10.0 milestone Sep 27, 2016

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