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

fix(angular): fix path for secondary entrypoints #2498

Merged
merged 1 commit into from
May 6, 2020

Conversation

viceice
Copy link
Contributor

@viceice viceice commented Feb 17, 2020

Secondary entrypoint path need to be adjusted to use the prebuild dist.

Current Behavior (This is the behavior we have today, before the PR is merged)

Secondary entrypoints are not adjusted.

{
 "paths": {
    "@nx-test/liba": ["dist/libs/liba"],
    "@nx-test/liba/sub": ["libs/liba/sub/src/index.ts"]
  }
}

Expected Behavior (This is the new behavior we can expect after the PR is merged)

Secondary entrypoints should be adjusted.

{
 "paths": {
    "@nx-test/liba": ["dist/libs/liba"],
    "@nx-test/liba/sub": ["dist/libs/liba/sub"]
  }
}

Issue

#1765

@viceice viceice marked this pull request as ready for review February 17, 2020 10:53
@viceice
Copy link
Contributor Author

viceice commented Feb 17, 2020

I need some help to add a proper test case

@vsavkin
Copy link
Member

vsavkin commented Feb 20, 2020

Hi! Thank you for your PR.

I reworked this, so there is now one function responsible for this logic for all builders. You will have to move the logic there:

49168c1#diff-41cb23901d7ec5f38adfd254146e8145R100

Regarding testing:
We angular-package.test.ts and react-package.test.ts. You can update the angular one to add a secondary entry point.

@viceice
Copy link
Contributor Author

viceice commented Feb 20, 2020

Ok, but the genral logic is fine?

@viceice
Copy link
Contributor Author

viceice commented Feb 20, 2020

@vsavkin Ready

@viceice
Copy link
Contributor Author

viceice commented Feb 26, 2020

unclear why tests failed 🤔

@vsavkin
Copy link
Member

vsavkin commented Mar 6, 2020

@viceice I think the implementation isn't right. Left a comment.

@viceice
Copy link
Contributor Author

viceice commented Mar 23, 2020

@vsavkin Ready for another review

@vsavkin
Copy link
Member

vsavkin commented Mar 23, 2020

One small comment. Other than that, it looks good.

@viceice
Copy link
Contributor Author

viceice commented Mar 23, 2020

Done

@viceice
Copy link
Contributor Author

viceice commented Mar 23, 2020

Any hints why tests fail again?

@brandonroberts
Copy link
Contributor

@viceice will you rebase this PR on master, and revert the changes to the scripts?

@brandonroberts brandonroberts self-assigned this Apr 6, 2020
@viceice
Copy link
Contributor Author

viceice commented Apr 6, 2020

Ok

@viceice
Copy link
Contributor Author

viceice commented Apr 7, 2020

@brandonroberts Done.

@brandonroberts
Copy link
Contributor

Will you rebase on master again to fix the merge conflicts? Thanks

@viceice
Copy link
Contributor Author

viceice commented May 6, 2020

@brandonroberts fixed the conficts. Should i rebase and force push?

@brandonroberts
Copy link
Contributor

@viceice Yes

@viceice viceice force-pushed the fix/secondary-entrypoints branch from 0e833fe to a9fd0d5 Compare May 6, 2020 14:46
@viceice
Copy link
Contributor Author

viceice commented May 6, 2020

@brandonroberts done

@brandonroberts brandonroberts merged commit 69faaed into nrwl:master May 6, 2020
@brandonroberts
Copy link
Contributor

Thanks @viceice

@viceice viceice deleted the fix/secondary-entrypoints branch May 7, 2020 04:20
@github-actions
Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 22, 2023
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.

None yet

3 participants