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

bugfix(transform): Disable handlebars transform pre-Ember-1.13 #150

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Nov 1, 2017

Followup to #149, this disables all handlebars transforms in pre-1.13 Ember. The transforms break otherwise, so this is a way to allow users to still get the benefits of test selectors, even if they won't be stripped.

@Turbo87
Copy link
Collaborator

Turbo87 commented Nov 1, 2017

Can you enable CI testing for 1.12?

@pzuraq
Copy link
Contributor Author

pzuraq commented Nov 1, 2017

Hmm, I assumed that tests would have had to be rewritten for older Ember but it looks like everything just works! 😄 updated

@marcoow
Copy link
Member

marcoow commented Nov 2, 2017

@pzuraq: unfortunately that's only because we're currently missing an acceptance test for that functionality. I'm adding on in #152. I guess we should merge that first, rebase this and make whatever changes are necessary (which is probably not running that particular test for Ember < 1.13).

@marcoow
Copy link
Member

marcoow commented Nov 2, 2017

@pzuraq: #152 was merged

@pzuraq pzuraq force-pushed the bugfix/turn-off-transform-for-older-versions-of-ember branch from e6b65e6 to 1ddaec0 Compare November 2, 2017 20:46
@pzuraq
Copy link
Contributor Author

pzuraq commented Nov 2, 2017

So I rebased and reran the tests locally. I thought it was weird because I expected some of the tests to fail, upon closer inspection several of them should have. It appears that because this addon may be triggering a bug in ember-cli-version-checker/ember-try where it reports the wrong Ember version. I'm not sure exactly what's going on here, but running ember try:one ember-1.11.4 --skip-cleanup locally resulted in ember-source not being removed from package.json.

I manually removed it and verified that tests failed without the changes from ember-compatibility-helpers.

@pzuraq
Copy link
Contributor Author

pzuraq commented Nov 2, 2017

Also worth noting, with the minimal examples in tests 1.11 actually does work. Dug into this locally and it seems like only certain block statements will trigger the bug, specifically link-tos, but others could exist.

For our purposes, I'd still prefer outright disabling the transforms. We'll be finishing the update to 1.13 soon, so this is just a temp fix to get us over the finish line

test('it transforms data-test params to hash pairs on components', function(assert) {
assert.equal(find('.test11').find('div[data-test-something]').length, 1, 'data-test-something exists');
});
test('it transforms data-test params to hash pairs on components', function(assert) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be the only test that fails for Ember < 1.13?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think @pzuraq might be right. Using the versionCompat feature of ember-try and ember-source together has some issues at the moment, and we might only be testing against ember-source in all scenarios 😱

Copy link
Member

Choose a reason for hiding this comment

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

Hm, so "best" solution for now might be to remove this one test for now (as well as investigating the bug and fixing it in ember-try)?

Copy link
Contributor Author

@pzuraq pzuraq Nov 3, 2017

Choose a reason for hiding this comment

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

The whole block of tests I commented out will fail for Ember <1.13 because positional params didn’t exist yet. They passed before because the transform was working (due to incorrect Ember version being detected) and changing the positional params into named params.

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.

3 participants