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.

Sign up for GitHub

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

Jump to bottom

Remove unnecessary dependencies from released NPM package #968

Merged
merged 4 commits into from
Nov 11, 2017
Merged

Remove unnecessary dependencies from released NPM package #968

merged 4 commits into from
Nov 11, 2017

Conversation

tricknotes
Copy link
Contributor

@tricknotes tricknotes commented Oct 28, 2017 •
edited by justin808
Loading

These dependencies are injected by generator spec.
However they are unnecessary for released react-on-rails package.

Even if the dependencies section is removed, it will back after $ rspec spec/react_on_rails/generators/install_generator_spec.rb is run.

I think it is unexpected behavior.
So I removed these dependencies and fix specs.

@Judahmeek @justin808
Can you review this PR? It this a correct way?

This change is Reviewable

@tricknotes tricknotes mentioned this pull request Oct 28, 2017
Closed
@tricknotes tricknotes changed the title Remove unnecessary dependencies from released package Remove unnecessary dependencies from released NPM package Oct 28, 2017
@tricknotes tricknotes force-pushed the remove-unnecessary-dependencies-from-released-package branch from c26e6cb to 33a8739 Compare October 28, 2017 16:23
@justin808
Copy link
Member

@tricknotes Can you please rebase on top of master. Then let's see the CI results.

@tricknotes tricknotes force-pushed the remove-unnecessary-dependencies-from-released-package branch from 33a8739 to b0697bb Compare October 29, 2017 01:47
@tricknotes
Copy link
Contributor Author

rebased

@tricknotes
Copy link
Contributor Author

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

https://travis-ci.org/shakacode/react_on_rails/jobs/294304640

hmm..., some tests are timeouted :<

@tricknotes tricknotes force-pushed the remove-unnecessary-dependencies-from-released-package branch from b0697bb to d888926 Compare October 29, 2017 12:39
@tricknotes tricknotes changed the title Remove unnecessary dependencies from released NPM package [WIP] Remove unnecessary dependencies from released NPM package Nov 7, 2017
@tricknotes tricknotes force-pushed the remove-unnecessary-dependencies-from-released-package branch 3 times, most recently from 6aea232 to fab1996 Compare November 9, 2017 01:11
@tricknotes tricknotes changed the title [WIP] Remove unnecessary dependencies from released NPM package Remove unnecessary dependencies from released NPM package Nov 9, 2017
@tricknotes
Copy link
Contributor Author

@justin808
Now all tests are passed on Travis CI.
Could you review this PR?

@justin808
Copy link
Member

This looks solid to me. @Judahmeek?

@Judahmeek
Copy link
Contributor

Nice work. However, I think that react-redux should at least be a peer dependency since it is referenced in ReactOnRails source code.

@tricknotes
Run generator inside destination root
b9e5e0c 
It is necessary to inform yarn what current directory should destination root.
@tricknotes
Always generate package.json for generator spec
1ed648c 
Without package.json in dummy app, yarn will search package.json in
ancestral directories.
And when running `yarn install <package-name>`, the package.json will
be destructively changed.
@tricknotes
Remove unnecessary dependencies from released NPM package
7ccac2c 
These dependencies were injected by generator spec.
They are unnecessary for released react-on-rails package.
@tricknotes
Fix .eslintrc to resolve react-redux
cb35cc4 
react-on-rails no longer depends on react-redux.
The template file that uses react-redux should be passed ESLint
regardless its dependency.
@tricknotes
Copy link
Contributor Author

Thanks for your review, @Judahmeek !
Yes, react-redux is referenced by ReactOnRails.
However it seems from specs, generators and docs, not from production code.
Additionally, the ReactOnRails users who use react without redux don't need react-redux.

I think it may be a optional dependencies. Or I've misunderstand?

@tricknotes tricknotes force-pushed the remove-unnecessary-dependencies-from-released-package branch from 30b4a9f to cb35cc4 Compare November 10, 2017 04:56
@Judahmeek
Copy link
Contributor

You're correct that it is an optional dependency.

@justin808 justin808 merged commit 45fbc21 into shakacode:master Nov 11, 2017
@tricknotes tricknotes deleted the remove-unnecessary-dependencies-from-released-package branch November 11, 2017 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tricknotes @justin808 @Judahmeek