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

WIP: Include peers #490

Merged
merged 4 commits into from Feb 28, 2017

Conversation

Projects
None yet
3 participants
@helfer
Copy link
Member

helfer commented Feb 27, 2017

TODO:

  • If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass
  • Update CHANGELOG.md with your change
  • If this was a change that affects the external API, update the docs and post a link to the PR in the discussion

@helfer helfer added the in progress label Feb 27, 2017

helfer added some commits Feb 27, 2017

@helfer helfer merged commit 63c6e05 into master Feb 28, 2017

5 checks passed

./dist/index.min.js No change (28,043 bytes)
CLA Author has signed the Meteor CLA.
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.06%) to 94.0%
Details

@helfer helfer deleted the include-peers branch Feb 28, 2017

@helfer helfer removed the in progress label Feb 28, 2017

@jaydenseric

This comment has been minimized.

Copy link
Contributor

jaydenseric commented Mar 1, 2017

@helfer After updating to v0.13.0, ApolloClient or gql are not available for import. They simply don't appear in the compiled dist.

@helfer

This comment has been minimized.

Copy link
Member Author

helfer commented Mar 1, 2017

@jaydenseric Yeah, I figured that out today as well. I'll have to dig into it.

@helfer

This comment has been minimized.

Copy link
Member Author

helfer commented Mar 1, 2017

@jaydenseric actually, I'm not sure why dist is included, because in package.json main is specified as lib/index.js. My guess as to why the exports are not included is that maybe the bundler uses the browser field instead, which is lib/browser.js, but as far as I can tell react-apollo's build setup doesn't actually write to that file!

cc @calebmer @jbaxleyiii

@jaydenseric

This comment has been minimized.

Copy link
Contributor

jaydenseric commented Mar 1, 2017

I can't be much help here because it's TypeScript :(

While messing around with this stuff, what are the chances of adding a package.json module entry for modern tools (such as Webpack 2) to consume ESM?

@jaydenseric

This comment has been minimized.

Copy link
Contributor

jaydenseric commented Mar 1, 2017

Created a proper issue regarding the module enhancement: apollographql/apollo#78.

@calebmer

This comment has been minimized.

Copy link
Contributor

calebmer commented Mar 1, 2017

Yeah, I’m not sure how the build is setup in react-apollo. I need to take a look at the builds for both apollo-client and react-apollo 😣

@helfer

This comment has been minimized.

Copy link
Member Author

helfer commented Mar 1, 2017

@calebmer: please focus on react-apollo first, because the build here seems somewhat broken while for Apollo Client it's still working.

@calebmer

This comment has been minimized.

Copy link
Contributor

calebmer commented Mar 1, 2017

@helfer do we want to schedule this as a fix for 1.0?

@helfer

This comment has been minimized.

Copy link
Member Author

helfer commented Mar 1, 2017

Yes, for this repo definitely.

@helfer helfer referenced this pull request Mar 3, 2017

Merged

Update browser.ts #501

@helfer

This comment has been minimized.

Copy link
Member Author

helfer commented Mar 3, 2017

@calebmer I was stupid and should have put the exports in browser.ts instead of index.ts. Feel free to ignore what I said before.

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