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

CIF-2499: use file dependencies instead of links to link npm packages #724

Closed
wants to merge 2 commits into from

Conversation

buuhuu
Copy link
Contributor

@buuhuu buuhuu commented Oct 22, 2021

Description

This replaces the usage of npm link with file:.. dependencies in the npm projects we are using. It still generates a symlink when running npm install but prevents the same command to interfere with already linked dependencies.

It removes also the npm link (installing the link locally in the frontend-maven-plugin) as it is not needed anymore.

Furthermore it removes the redundant build of the react-components in example/ui.frontend to align with the behaviour of the product-recs/react-components.

Related Issue

CIF-2499

Motivation and Context

Build improvements

How Has This Been Tested?

Locally, CI

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes and the overall coverage did not decrease.
  • All unit tests pass on CircleCi.
  • I ran all tests locally and they pass.

@codecov
Copy link

codecov bot commented Oct 22, 2021

Codecov Report

Merging #724 (59a0421) into master (1b2496a) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #724   +/-   ##
=========================================
  Coverage     88.70%   88.70%           
  Complexity     1706     1706           
=========================================
  Files           294      294           
  Lines          7465     7465           
  Branches       1095     1095           
=========================================
  Hits           6622     6622           
  Misses          629      629           
  Partials        214      214           
Flag Coverage Δ
integration 58.25% <ø> (ø)
jest 86.15% <ø> (ø)
karma 89.86% <ø> (ø)
unittests 88.83% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b2496a...59a0421. Read the comment docs.

@herzog31
Copy link
Member

I think this is related to npm/cli#2147. Deleting the package-lock.json files of all react projects and re-creating them using npm install should fix the problem. It worked for me locally.

The problem happened to me basically when doing the following sequence in examples/ui.frontend:

  • npm install
  • npm link @adobe/aem-core-cif-react-components
  • npm install <- this installs the dependencies of the core react components project again, but does not create the bin folder.

@buuhuu
Copy link
Contributor Author

buuhuu commented Oct 25, 2021

You mean doing a npm install on the react-components again?

For me that also created the .bin directory but formartjs cli is missing. Actually this is the behaviour that we had before as the react-components maven module is built before the examples ui.frontend so executes the npm install.

@herzog31
Copy link
Member

You mean doing a npm install on the react-components again?

No, I performed all operations in the examples/ui.frontend project.

When doing npm install in the core react components project again, it will not re-create the bin folder. It seems npm isn't smart enough to notice it is missing. It will only be re-created if the node_modules folder is re-created from scratch.

@buuhuu
Copy link
Contributor Author

buuhuu commented Oct 25, 2021

In #727 I recreated the package-lock.json(s).

I seems that the examples/ui.frontend package-lock.json still contained the dependencies from the time before we made them peer dependencies. Maybe that is related to the issue.

@buuhuu buuhuu changed the title CIF-2449: use file dependencies instead of links to link npm packages CIF-2499: use file dependencies instead of links to link npm packages Oct 26, 2021
@buuhuu buuhuu closed this Oct 26, 2021
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.

3 participants