Skip to content
This repository was archived by the owner on Sep 7, 2022. It is now read-only.

Conversation

stevemao
Copy link
Member

@stevemao stevemao commented Jul 28, 2016

chore(deps): add missing devdeps
feat(deps): require react and reactDOM since they are peerdeps

@stevemao
Copy link
Member Author

stevemao commented Jul 29, 2016

Travis failed because of rollup config (since it actually thinks it's importing react). Should bundle only happen on the application (top) level?

@treshugart
Copy link
Contributor

Bundle needs to happen on prepublish. You can modify the rollup config (module.exports in rollup.config.js) to have externals for React and ReactDOM so it won't try and bundle them. That might work.

@treshugart
Copy link
Contributor

Hey @stevemao just got to merging your other PRs. Can you have another look at this one when you have a chance pls? Thanks!

@stevemao
Copy link
Member Author

Yes, of course.

* master:
  fix(context): bind the right context to prototype functions
  feat(proto): expose web component proto methods onto react component
  Revert "test(proto): add a test for props on prototype"
  Revert "feat(proto): expose web component proto props onto react component"
  test(proto): add a test for props on prototype
  feat(proto): expose web component proto props onto react component
  fix(event): on[upper]* for event name syntax
  docs(README): clarify react component
@@ -1 +1,13 @@
module.exports = require('skatejs-build/rollup.config');
const config = require('skatejs-build/rollup.config');
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Do you think it'd be a good idea to ignore peer dependencies here, as well? https://github.com/skatejs/build/blob/master/rollup.config.js#L24

Copy link
Member Author

@stevemao stevemao Aug 31, 2016

Choose a reason for hiding this comment

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

why? skatejs-build doesn't have peer dependencies?

Copy link
Member Author

@stevemao stevemao Aug 31, 2016

Choose a reason for hiding this comment

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

@treshugart should we put this logic in https://github.com/skatejs/build?

Copy link
Contributor

Choose a reason for hiding this comment

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

I know it doesn't have peer deps, but the build uses the cwd.

Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be ideal to be put in there, I think. If you want to do that as part of this, I think that would be best.

@bradleyayers
Copy link
Member

I don't have a lot of context to this but it looks pretty weird. It looks like it should work though.

@bradleyayers
Copy link
Member

lgtm

@treshugart
Copy link
Contributor

@bradleyayers the idea is that react and react dom can be required as they'd be peer dependencies and provided by the consumer anyways. This build stuff can eventually be contained in skatejs/build (as it already handles dependencies, just not peerDependencies).

@treshugart
Copy link
Contributor

lgtm

@stevemao
Copy link
Member Author

LGTM

@stevemao
Copy link
Member Author

lgtm

@treshugart
Copy link
Contributor

lgtm!

@treshugart treshugart merged commit 567175d into webcomponents:master Aug 31, 2016
@bradleyayers
Copy link
Member

lgtm

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.

3 participants