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

Changed build to run all npm builds before backend builds. #5433

Merged
merged 1 commit into from
Dec 13, 2022

Conversation

valadas
Copy link
Contributor

@valadas valadas commented Dec 11, 2022

This makes yarn install and yarn build before any project builds. It still uses lerna as before.

  • I confirmed that a failed install makes the build fail.
  • I confirmed that a failed npm build makes the build fail.
  • I confirmed that it fixed the resource manager scripts missing from the final build.

The drawback of this approach is that one can no longer build only the persona bar in release mode without using cake. But for dev/debug, the current documented way with yarn start --scope ... is still valid.

This makes `yarn install` and `yarn build` before any project builds. It still uses lerna as before.

- I confirmed that a failed install makes the build fail.
- I confirmed that a failed npm build makes the build fail.
- I confirmed that it fixed the resource manager scripts missing from the final build.

The drawback of this approach is that one can no longer build only the persona bar in release mode without using cake. But for dev/debug, the current documented way with `yarn start --scope ...` is still valid.
@valadas valadas added this to the 9.11.1 milestone Dec 11, 2022
Copy link
Contributor

@david-poindexter david-poindexter left a comment

Choose a reason for hiding this comment

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

Thank you @valadas - this looks like the lessor of evils here.

@valadas
Copy link
Contributor Author

valadas commented Dec 11, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Comment on lines +503 to +505
ProjectSection(ProjectDependencies) = postProject
{9CCA271F-CFAA-42A3-B577-7D5CBB38C646} = {9CCA271F-CFAA-42A3-B577-7D5CBB38C646}
EndProjectSection
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know what this change is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no clue, it was done by Visual Studio...

@@ -1,14 +1,12 @@
<Project ToolsVersion="4.0" DefaultTargets="Build"
xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<Import Project="..\..\packages\Yarn.MSBuild.1.22.17\build\Yarn.MSBuild.props" Condition="Exists('..\..\packages\Yarn.MSBuild.1.22.17\build\Yarn.MSBuild.props')" />
<Import Project="..\..\packages\Yarn.MSBuild.1.22.17\build\Yarn.MSBuild.targets" Condition="Exists('..\..\packages\Yarn.MSBuild.1.22.17\build\Yarn.MSBuild.targets')" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove the Yarn.MSBuild package entirely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, if we go with this solution then it is unused...

Copy link
Contributor

@donker donker left a comment

Choose a reason for hiding this comment

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

I quite like moving the npm build from being hidden in some build file to the main scripts

@donker donker merged commit 088dada into dnnsoftware:develop Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants