Skip to content
This repository has been archived by the owner on Apr 13, 2023. It is now read-only.

Better packaging utilizing package.json browser field #306

Merged
merged 1 commit into from
Nov 23, 2016

Conversation

larixer
Copy link
Contributor

@larixer larixer 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.

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.
@larixer larixer changed the title Better packaging Better packaging utilizing package.json browser field Nov 3, 2016
@larixer larixer mentioned this pull request Nov 3, 2016
@tmeasday
Copy link
Contributor

@jbaxleyiii are you OK with these changes?

@jbaxleyiii
Copy link
Contributor

@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
Copy link
Contributor

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

@larixer
Copy link
Contributor Author

larixer commented Nov 14, 2016

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

@larixer
Copy link
Contributor Author

larixer 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
Copy link
Contributor

Alright I'm game then!

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

Thanks for working hard to get this through @Vlasenko

@tmeasday
Copy link
Contributor

@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 :/

@larixer
Copy link
Contributor Author

larixer 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)

@larixer
Copy link
Contributor Author

larixer commented Nov 23, 2016

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

@tmeasday
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
Copy link
Contributor

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

@larixer
Copy link
Contributor Author

larixer commented Nov 23, 2016

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

@tmeasday
Copy link
Contributor

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 subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants