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

Cleanup dependencies. #73

Closed
wants to merge 11 commits into from
Closed

Cleanup dependencies. #73

wants to merge 11 commits into from

Conversation

mshima
Copy link
Member

@mshima mshima commented Oct 4, 2021

@mshima mshima marked this pull request as ready for review October 5, 2021 01:56
@pascalgrimaud
Copy link
Member

We need your reviews / tests if possible plz, @deepu105 @ctamisier @Jackjun724

@ctamisier
Copy link
Contributor

LGTM

I've tried the following steps.

First:
npm install -g npm@latest

Then in react-jhipster:
npm install -> works
npm test -> works
npm ci -> works
npm run build -> works

Then in the host project (e.g. generated app):
npm install ../react-jhipster/react-jhipster-0.16.2.tgz (after npm pack in react-jhipster)
npm install -> works
npm test -> works
npm run webapp:build -> works

If I drop peerDependencies in react-jhipster, I have the same exact behavior (Maybe I'm missing something..).
About the potential jest conflict (I don't know how to "reproduce" it), maybe there is no problem as it is a devDependency ?(and not a dependency).. BTW I'm far to understand everything about npm and its dependency management

@Jackjun724
Copy link
Contributor

Jackjun724 commented Oct 5, 2021

It seems that there is no problem, but I think the root cause of the problem is react-transition-group is tool dependency instead of ​core dependency, So it shouldn't be peerDependency, jest it seems to be a potential conflict.

Overall, it works well :)

@mshima
Copy link
Member Author

mshima commented Oct 5, 2021

If I drop peerDependencies in react-jhipster, I have the same exact behavior (Maybe I'm missing something..).
About the potential jest conflict (I don't know how to "reproduce" it), maybe there is no problem as it is a devDependency ?(and not a dependency).. BTW I'm far to understand everything about npm and its dependency management

@ctamisier remove peerDependencies and try to recreate the package-lock.json with npm@7 and it will show peer conflicts.

rm -rf node_modules package-lock.json
npx npm@7 install # if you have npm@6 globally

react-transition-group was removed from package.json. It isn't used directly, it's a transitive dependency of reactstrap.

@ctamisier
Copy link
Contributor

I don't get it as we don't have the same result. I'll keep investigating.
But for the release I'd say everything is ok.

image

@mshima
Copy link
Member Author

mshima commented Oct 5, 2021

I don't get it as we don't have the same result. I'll keep investigating. But for the release I'd say everything is ok.

Looks like are using my branch or another one, see the react-transition-group that existing in main and doesn't appears at the diff.

"react-transition-group": "^4.4.1",

@mshima
Copy link
Member Author

mshima commented Oct 5, 2021

@ctamisier are you proposing to drop peer dependencies?
IMO it's important to keep it for derived projects.

@mshima
Copy link
Member Author

mshima commented Oct 5, 2021

I've reverted the bundle change, so we don't need to bump to 0.17.x

Is bundles used somewhere?
IMO we should drop it, bundles have been replaced with tree-shaking.
The cleaner a project is, the easier is to maintain.

@ctamisier
Copy link
Contributor

@ctamisier are you proposing to drop peer dependencies?
IMO it's important to keep it for derived projects.

Removing peerDependencies on your branch is working.
Removing peerDependencies on main is failing (but it was also failing when we keep them (fails only when doing rm -rf node_modules package-lock.json)

I think you know way more than me about all this, so yes let's keep peerDependencies. (I really don't know the impact by dropping them, just because of my limited knowledge about this). I was just seeing that removing peerDependenciies was also working on your branch.

@mshima mshima mentioned this pull request Oct 5, 2021
Copy link
Member

@deepu105 deepu105 left a comment

Choose a reason for hiding this comment

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

Looks good. If those deps are not required anymore. Keeping peer deps would be a good idea

@mshima mshima closed this in #74 Oct 5, 2021
@mshima mshima deleted the deps branch October 5, 2021 16:50
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.

5 participants