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] Export more public API's via modules #4125

Merged
merged 1 commit into from
Feb 12, 2016
Merged

[BUGFIX beta] Export more public API's via modules #4125

merged 1 commit into from
Feb 12, 2016

Conversation

pangratz
Copy link
Member

This change allows to import more public API's via modules:

  • import Serializer from "ember-data/serializer"
  • import EmbeddedRecords from "ember-data/serializers/embedded-records-mixin"

Should this be [BUGFIX release]?

cc @fivetanley @bmac

@rwjblue
Copy link
Member

rwjblue commented Jan 29, 2016

We started with these alias type of modules for the early 2.3.0-betas, but ended up having issues and changing things around to move the actual module to the new public location (instead of shim modules).

IMHO, we should be consistent in our public modules. So either they existing ones should be made into shims, or these should be moving the underlying modules and avoid shims...

@fivetanley
Copy link
Member

I am all for avoiding shims.

@bmac
Copy link
Member

bmac commented Jan 29, 2016

Are BooleanTransform and friends typically used in app code in a way where they would need to be imported?

@bmac
Copy link
Member

bmac commented Jan 29, 2016

I don't think this needs to be a [BUGFIX release] unless these paths were previously supported by https://github.com/ember-cli/ember-cli-shims and their omission is breaking existing applications.

@pangratz
Copy link
Member Author

pangratz commented Feb 6, 2016

Are BooleanTransform and friends typically used in app code in a way where they would need to be imported?

You are right, they might not be needed as public API. I removed the public API modules for the transforms.

@@ -0,0 +1,3 @@
import Serializer from "ember-data/-private/system/serializer";

export default Serializer;
Copy link
Member

Choose a reason for hiding this comment

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

@pangratz could you move the Serializer and EmbeddedRecordsMixin out of the -private folder and into their new public locations.

@pangratz
Copy link
Member Author

pangratz commented Feb 8, 2016

I moved the files from the private location to the public ones.

Also, the embedded records mixin is now located at ember-data/serializers/embedded-records-mixin. As brought up for discussion by @bmac, I think this path might be better since it clarifies that the mixin should be used in combination with a serializer...


test("ember-data/serializers/rest", function(assert) {
assert.ok(RESTSerializer);
});
Copy link
Member

Choose a reason for hiding this comment

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

Yay Tests!

@bmac
Copy link
Member

bmac commented Feb 12, 2016

👍 looks good to me. Just needs a rebase.

This change allows to import more public API's via modules:

- import Serializer from "ember-data/serializer"
- import EmbeddedRecords from "ember-data/serializers/embedded-records-mixin"
@pangratz
Copy link
Member Author

@bmac done

bmac added a commit that referenced this pull request Feb 12, 2016
[BUGFIX beta] Export more public API's via modules
@bmac bmac merged commit 03ceeb5 into emberjs:master Feb 12, 2016
@pangratz pangratz deleted the more-public-modules branch February 12, 2016 08:05
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

4 participants