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

Update bundling scheme #347

Merged
merged 3 commits into from
Mar 24, 2017
Merged

Update bundling scheme #347

merged 3 commits into from
Mar 24, 2017

Conversation

claydiffrient
Copy link
Contributor

This commit sets things up to work properly when
publishing to npm. Prior to publishing the package
items from the src/ folder will be processed with Babel
and stored in the lib/ folder. The lib/ folder will
not be present in the repository. The src/ folder similarly
will not be available when looking at the installed package.

In addition this commit also fixes #332 by adding the
babel-plugin-transform-object-assign plugin and removing
lodash.assign

Changes proposed:

  • lib/ renamed to src/ in repo
  • src/ transpiled through Babel to lib on publish
  • lodash.assign removed, in favor of babel-plugin-transform-object-assign

Acceptance Checklist:

  • All commits have been squashed to one.
  • The commit message follows the guidelines in CONTRIBUTING.md.
  • Documentation (README.md) and examples have been updated as needed.
  • If this is a code change, a spec testing the functionality has been added.
  • If the commit message has [changed] or [removed], there is an upgrade path above.

This commit sets things up to work properly when
publishing to npm.  Prior to publishing the package
items from the src/ folder will be processed with Babel
and stored in the lib/ folder. The lib/ folder will
not be present in the repository.  The src/ folder similarly
will not be available when looking at the installed package.

In addition this commit also fixes #332 by adding the
`babel-plugin-transform-object-assign` plugin and removing
`lodash.assign`
@diasbruno
Copy link
Collaborator

diasbruno commented Mar 5, 2017

This is what I got with commit 96cc7ae.

ERROR in ./src/index.js
Module not found: Error: Can't resolve 'react-modal' in '/projects/react-modal-qa/src'
 @ ./src/index.js 13:18-40
 @ multi (webpack)-dev-server/client?http://0.0.0.0:8080 ./src/index.js
webpack: Failed to compile.

react-modal-qa to make things easier to test.

{
    "react-modal": "git@github.com:reactjs/react-modal.git#build/reorganization"
}

@claydiffrient
Copy link
Contributor Author

Hmm... I bet I missed something that specifically targets src/ in the transfer. I'll have to look more into that. There's some linting things I've got to fix up too.

@diasbruno
Copy link
Collaborator

Probably this package.json.

{
  "main": "./lib/index.js"
}

@diasbruno diasbruno force-pushed the build/reorganization branch 4 times, most recently from 220997c to daec036 Compare March 5, 2017 20:16
@diasbruno
Copy link
Collaborator

Even after added "babel-transform-class-properties" it keeps failing.

Hash: ebb4dcf49212a3d0a840
Version: webpack 2.2.1
Time: 13223ms
        Asset     Size  Chunks                    Chunk Names
./app/main.js  1.04 MB       0  [emitted]  [big]  main
chunk    {0} ./app/main.js (main) 1 MB [entry] [rendered]
   [26] ./~/react/lib/React.js 2.69 kB {0} [built]
  [113] ./src/index.js 3.05 kB {0} [built]
  [114] (webpack)-dev-server/client?http://0.0.0.0:8080 5.28 kB {0} [built]
  [140] ./~/querystring-es3/index.js 127 bytes {0} [built]
  [142] ./~/react-dom/index.js 59 bytes {0} [built]
  [214] ./~/react-modal/src/components/Modal.js 386 bytes {0} [built] [failed] [1 error]
  [215] ./~/react-modal/src/index.js 63 bytes {0} [built]
  [227] ./~/react/react.js 56 bytes {0} [built]
  [255] ./~/strip-ansi/index.js 161 bytes {0} [built]
  [257] ./~/url/url.js 23.3 kB {0} [built]
  [258] ./~/url/util.js 314 bytes {0} [built]
  [259] (webpack)-dev-server/client/overlay.js 3.6 kB {0} [built]
  [260] (webpack)-dev-server/client/socket.js 856 bytes {0} [built]
  [262] (webpack)/hot/emitter.js 77 bytes {0} [built]
  [263] multi (webpack)-dev-server/client?http://0.0.0.0:8080 ./src/index.js 40 bytes {0} [built]
     + 249 hidden modules

ERROR in ./~/react-modal/src/components/Modal.js
Module parse failed: /projects/react-modal-qa/node_modules/react-modal/src/components/Modal.js Unexpected token (19:19)
You may need an appropriate loader to handle this file type.
|
|   /* eslint-disable react/no-unused-prop-types */
|   static propTypes ({=} THIS})  {
|     isOpen: React.PropTypes.bool.isRequired,
|     style: React.PropTypes.shape({
 @ ./~/react-modal/src/index.js 1:0-39
 @ ./src/index.js
 @ multi (webpack)-dev-server/client?http://0.0.0.0:8080 ./src/index.js
webpack: Failed to compile.

@claydiffrient
Copy link
Contributor Author

claydiffrient commented Mar 5, 2017 via email

@diasbruno
Copy link
Collaborator

I'll revert this changes and continue in another branch.

This temporarily disables react/require-default-props
until it can be addressed in a separate commit.
@coveralls
Copy link

Coverage Status

Coverage decreased (-2.9%) to 77.778% when pulling e3e177b on build/reorganization into a8b7aa0 on master.

@claydiffrient
Copy link
Contributor Author

I'm not sure where the issues are coming from @diasbruno. I set up a local testing repository and linked in my react-modal via yarn link and yarn link react-modal and things seem to work fine.

Oh I think I know why. In your react-modal-qa repo you are referencing the branch directly, but I believe that will not run the pre-publish hook to compile the code beforehand. So it'll never see the lib/ directory that gets created prior to publish. Would you mind trying it out using yarn/npm link and see if it works as you would expect?

@diasbruno
Copy link
Collaborator

I'll check it.

@diasbruno
Copy link
Collaborator

diasbruno commented Mar 22, 2017

Yep, that was the problem. Using npm link really executes the prepublish command...and worked fine.

@claydiffrient claydiffrient merged commit 9cb8441 into master Mar 24, 2017
@claydiffrient claydiffrient deleted the build/reorganization branch March 24, 2017 03:24
@diasbruno diasbruno mentioned this pull request Jun 14, 2017
5 tasks
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.

Reduce build size by swapping out lodash.assign for a minimal Object.assign
3 participants