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 BETA]: Fixed export regression. ember-data/transform to be default. #5008

Merged
merged 1 commit into from Jun 12, 2017

Conversation

workmanw
Copy link

@workmanw workmanw commented Jun 9, 2017

@stefanpenner made reference to this in the original PR (24f4120#commitcomment-22121947), but I think it was just overlooked. Either way this created a regression for us when using: import Transform from 'ember-data/transform';.

@@ -1 +1 @@
export { Transform } from './-private';
export { Transform as Default } from './-private';
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be lowercase?

Copy link
Author

Choose a reason for hiding this comment

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

😬 Yea ... I'm not sure how that worked. I guess the tests don't exercise that.

@workmanw workmanw force-pushed the transform-default-export branch 3 times, most recently from 9e05cc5 to 2f64d11 Compare June 9, 2017 21:18
@workmanw
Copy link
Author

workmanw commented Jun 9, 2017

~~@stefanpenner So it looks like the transform code was moved again after the first commit. Not it resides in ember-data/transforms/transform, but we also want to map it to ember-data/transform.~~~

```
export * from './transforms/transform';
```
~~~Alternative I think I could have done:~~~
```
export { default } from './transforms/transform';
```
~~Let me know if you have a preference.~~

Edit: GAH. It seems like `export * from './transforms/transform';` doesn't actually export the default. 🤦‍♂️ . We'll have to go with the later.

@workmanw
Copy link
Author

workmanw commented Jun 9, 2017

Err. Actually I'm not actually sure if export { default } from './transforms/transform'; is valid. It may happen to just work because of how babel transpiles. It's unclear to me by looking at the spec ( https://tc39.github.io/ecma262/#sec-exports ). @stefanpenner do you know by chance? Worst case is I could import with one statement and reexport default as a second.

Who knew one line of code could be so complicated 😜

@runspired
Copy link
Contributor

@workmanw this is fixed elsewhere I thought it landed

@workmanw
Copy link
Author

workmanw commented Jun 10, 2017

@runspired Hmm. It's not in master or beta. I also checked the recent most PRs. I don't see any sign of it. Maybe it was in a close (unmerged) PR?

EDIT: One thing I did notice is that the transforms were briefly in the -private package. And now they're not. So maybe in all of that shuffling, the export from -private/index.js was removed and that's what caused this breakage?

@bmac
Copy link
Member

bmac commented Jun 12, 2017

Thanks @workmanw.

@bendemboski
Copy link

bendemboski commented Jun 18, 2017

The bug that this PR fixed just got released in 2.14.0, but this fix isn't in that release, it's in the 2.15 track, so this regression exists in the latest release ☹️

@bmac
Copy link
Member

bmac commented Jun 18, 2017

Thanks for the heads up @bendemboski. I missed this when cherry picking commits for the release. I'll release a 2.14.1 with this commit as soon as I get a chance.

@bmac
Copy link
Member

bmac commented Jun 19, 2017

Ember Data 2.14.1 has been published with this fix.

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.

None yet

5 participants