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

Better packaging utilizing package.json browser field #306

Merged
merged 1 commit into from Nov 23, 2016

Conversation

Projects
None yet
4 participants
@vlasenko
Copy link
Contributor

vlasenko commented Nov 3, 2016

Provide compiled sources from 'lib' for node modules. Provide all API exports from index.js.

This is breaking change.

As a side effect this change fixes file paths in Stack Traces when react-apollo throws exceptions.

Better packaging
Provide compiled sources from 'lib' for node modules. Provide all API exports from index.js.

This is breaking change.

As a side effect this change fixes file paths in Stack Traces when react-apollo throws exceptions.

@vlasenko vlasenko force-pushed the vlasenko:fix-packaging-cc branch from ade0c3e to 6edab0c Nov 3, 2016

@vlasenko vlasenko changed the title Better packaging Better packaging utilizing package.json browser field Nov 3, 2016

@vlasenko vlasenko referenced this pull request Nov 3, 2016

Closed

Better packaging #301

@tmeasday

This comment has been minimized.

Copy link
Contributor

tmeasday commented Nov 14, 2016

@jbaxleyiii are you OK with these changes?

@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Nov 14, 2016

@tmeasday I'm still concerned with the over shipping to clients. Not all packages respect the browser field in package.json. (AFAIK Meteor doesn't even support it?). I don't think it's worth the potential extra shipping to save a / in the import?

@stubailo

This comment has been minimized.

Copy link
Member

stubailo commented Nov 14, 2016

Meteor definitely honors the browser field! AFAIK Webpack and Browserify do too. I don't know about stuff like Rollup.

@vlasenko

This comment has been minimized.

Copy link
Contributor Author

vlasenko commented Nov 14, 2016

@stubailo Rollup supports it too, in the node resolve plugin:
https://github.com/rollup/rollup-plugin-node-resolve

@vlasenko

This comment has been minimized.

Copy link
Contributor Author

vlasenko commented Nov 14, 2016

@jbaxleyiii

I don't think it's worth the potential extra shipping to save a / in the import?

Well, I'm not only trying to save '/' in the import.
The goals are the following:

  1. Fix Stack Traces.
  2. Discourage people to use private package methods, located in files after / in the import.
  3. Ship only relevant package files, don't ship things like scripts folder or .travis.yml file, that are non-relevant for npm module. But ship src folder contents, that is needed to understand stack traces.
  4. Moving compiled files outside of target compilation folder breaks TypeScript typings optimizations. If files were where they compiled to, we could provide typings for 3rd party libs, like redux compose (to not require all the redux lib, but only compose function). I will submit PR that does so, after this PR will be merged.
@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Nov 14, 2016

Alright I'm game then!

@tmeasday tmeasday merged commit 6edab0c into apollographql:master Nov 23, 2016

3 checks passed

CLA Author has signed the Meteor CLA.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 95.611%
Details
@tmeasday

This comment has been minimized.

Copy link
Contributor

tmeasday commented Nov 23, 2016

Thanks for working hard to get this through @vlasenko

@tmeasday

This comment has been minimized.

Copy link
Contributor

tmeasday commented Nov 23, 2016

@vlasenko any idea why master is now failing w/

Invariant Violation: ReactCompositeComponent: injectEnvironment() can only be called once.

It passed before I merged this PR and the PR itself passed :/

@vlasenko

This comment has been minimized.

Copy link
Contributor Author

vlasenko commented Nov 23, 2016

@tmeasday I think injectEnvironment is not related to this PR, but rather related to another PR and I have even left comment with caution about my observations:
#305 (comment)

@vlasenko

This comment has been minimized.

Copy link
Contributor Author

vlasenko commented Nov 23, 2016

@tmeasday Though at this stage, I think you can just upgrade everything to React@15.4 instead.

@tmeasday

This comment has been minimized.

Copy link
Contributor

tmeasday commented Nov 23, 2016

With 15.4 I started seeing a whole lot of warnings like:

jest-haste-map: duplicate manual mock found:
  Module name: ErrorUtils
  Duplicate Mock path: /Users/tom/Apollo/react-apollo/node_modules/react/node_modules/fbjs/lib/__mocks__/ErrorUtils.js
This warning is caused by two manual mock files with the same file name.
Jest will use the mock file found in:
/Users/tom/Apollo/react-apollo/node_modules/react/node_modules/fbjs/lib/__mocks__/ErrorUtils.js
 Please delete one of the following two files:
 /Users/tom/Apollo/react-apollo/node_modules/react-native/Libraries/Utilities/__mocks__/ErrorUtils.js
/Users/tom/Apollo/react-apollo/node_modules/react/node_modules/fbjs/lib/__mocks__/ErrorUtils.js

And ultimately errors like:

    Cannot find module 'ReactNativeDefaultInjection' from 'setup.js'

Simply bumping the react-test-renderer dep to the rc works for now, until we resolve the above.

@tmeasday

This comment has been minimized.

Copy link
Contributor

tmeasday commented Nov 23, 2016

Ahh, the problem was react-native doesn't yet support react@15.4, except in a pre-release. Bumping to that.

@vlasenko

This comment has been minimized.

Copy link
Contributor Author

vlasenko commented Nov 23, 2016

@tmeasday Yeah, somehow this all is not surprising :)
http://i.imgur.com/KzCHMAx.gifv

@tmeasday

This comment has been minimized.

Copy link
Contributor

tmeasday commented Nov 23, 2016

I blame npm, as always. Although in their defence I did ignore a peer dep warning (which they've conditioned me to do)

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