Fixes dnn-react-common build

[valadas]

Closes #3455

This is an alternate solution for PR #3456

What was done:
Problem 1 - linking
The main issue here was that the npm package for dnn-react-common was pulled even though we though it used the local package thanks to lerna and yarn workspaces, so local changes to the common components would not reflect in the modules that consumed it. I found out that when lerna uses yarn workspaces, it only uses the local dependencies if they are requested using their specific version and that version is higher than the npm published version. So the build scripts where updated to both set each package.json version to the current Dnn version and also sets all modules that load dnn-react-common to specifically request that exact version. This made the modules actually consume the local package. I also commited the package.json and yarn.lock with those new versions to avoid confusing and to prevent a future issue where we may have the same local version and published version, the version is FullSemVer which garatees we can never have the same version, as soon as the is any code change commited, the version can never be the same. Don't worry about the unstable version number, when we get to RC, this will automatically become something like 9.5.0-beta-1 and when we merge into master and tag v9.5.0 it will become simply 9.5.0 without anything else. Then back into the development branch (after releasing 9.5.0) it will become automatically 9.5.2-unstable-xxx

Problem 2 - parallelism
lerna --parallel option builds all packages in parallel ignoring all local dependency tree. I found out that the --stream option first build the dependency tree and then builds dependees first followed by the ones that nobody depends on in parallel. This previously caused some intermittent issues because of faster/slower build times on first try vs subsequent tries, it looks fine to me now with the --stream option.

Problem 3 - svg
raw-loader was updated between now and the last time the package was published to npm, we never noticed this but the way to load svgs inline has changed with that version and .default needed to be appended to each svg import declaration.

Problem 4 - odd stuff
There was two strange issues with not finding the cluster package (which we don't even use) and another one that I cannot remember now for the life of me. This was solved by @mtrutledge with a couple changes to targets and node-resolution changes also integrated into this PR.

To test this PR, I changed the Button common component by adding a test css class, ran build.ps1 installed a new site and notice by inspecting in the browser devtools that buttons do have that new test css class. I reverted that change to not commit it but just a quick way to ensure this now works.

[valadas]

Oh and for whoever merges this, please do squash and merge, I had to try a lot of commits before we got it right.

[mitchelsellers]

@valadas Which of these PR's should we take?

[mtrutledge]

I made a couple comments on a few files. I also don't see my package.json and lerna.json changes in this PR for the files changed. In order for lerna to build and hoist the proper items the packages need to be defined the same as the yarn workspaces. It also lets lerna know exactly where to go for its utility commands. If we dont want to use lerna at all that is an option but the commands need to be changed to 'yarn workspaces build'. Lerna gives the option to be able to easily switch to a different package manager if another one or better one comes out, etc.

[mtrutledge]

I think this should just call 'build' and the build script in the root package json should be updated. That way if another package replaces lerna or yarn workspaces becomes a bit more robust we just change the scripts in package.json and we keep everything javascript together. Then this AEModule.build file would not need to be changed.

[valadas]

The reason we brought Leena was for reducing build time using parallelism. either because of bad documentation or bad reading, the --stream option should have been used instead of --parallel it still uses yarn workspaces under to find the packages and build its dependency tree. Also listing the packages is not needed in the Leena file if they are defined in the workspace. This did not change anything and created duplication

[bdukes]

@valadas I don't think @mtrutledge is arguing against switching to --stream, he's just saying the command should be in the package.json instead of this MSBuild file. See https://github.com/dnnsoftware/Dnn.Platform/pull/3456/files#diff-b9cfc7f2cdf78a7f4b91a753d10865a2R30-R36, where the scripts section is added to package.json, so that the MSBuild file can just call yarn build

[mtrutledge]

Yes @bdukes and @valadas that is what I was saying.

[mtrutledge]

Without the packages in the lerna.json file, it will start to cause problems. This was one of the issues that was fixed in my PR that was causing the majority of the weirdnesses. I was able to get a few things working without updating it but eventually had to put in all the packages that were defined in workspaces to solve all the weirdnesses. Without it defined Lerna commands won't know exactly what to be looking for when looking at leaf dependencies. I think it is better to define them and keep them in sync then not have them in there at all.

According to the lerna documentation:

The packages config in lerna.json is a list of globs that match directories containing a package.json, which is how lerna recognizes "leaf" packages (vs the "root" package.json, which is intended to manage the dev dependencies and scripts for the entire repo).

[valadas]

Oh yeah sure. Wont be able to submit until late or tomorrow

[valadas]

The packages config in lerna.json is a list of globs that match directories containing a package.json, which is how lerna recognizes "leaf" packages (vs the "root" package.json, which is intended to manage the dev dependencies and scripts for the entire repo).

They should add "unless you use yarn workspaces" to that documentation... It uses what is defined in the yarn workspace if that option is enabled. So adding it in here would just duplicate that definition. Also the issue of build order was the --parallel option and the fact that we need to refer to specific versions for using the local packages, which the automation of versioning fixes too. You can run yarn workspace info to see what local packages are used. The only way that all dependents used the local code was setting them to the exact same version.

[valadas]

Also, the order they are defined does not mean much, they could stay in alphabetical order, lerna or yarn workspaces will build a dependency tree and build things in a order that makes sense, the problem was --parallel option was specifically saying to not care about the dependency tree and to build everything in parallel. With --steam it builds the dependees first and then the rest in parallel.

[valadas]

@valadas Which of these PR's should we take?

I would go with this one that includes the build fixes of the other one + automates versioning and fixes an svg issue.

[donker]

This works and does what it says it does

[valadas]

Ok, this was now rebased as per #3456 being merged and above comments.

Notes:

• In the dnn-react-common package.json, the version scripts where removed we will handle publishing in CI since this should not be done locally. We also tag manually now that we have merged this into the main project.
• Removed the packages definition in lerna.json, this is not needed when using the useWorkspaces option, lerna uses what is defined in the workspace, this would just duplicate the definition and not by DRY.
• In the main package.json, reordered packages alphabetically, this was not the reason of the issue, the reason was --parallel was overriding normal tree building to build everything in parallel no matter the dependency tree. That option was replaced by --stream which is what we should have used in the first place.

Small note, although #3456 did fix the issue for in-browser, this was still not a solution for npm consuming, this can be double-checked by running yarn workspaces info as stated above. In other words we would not have code completion, props validation, etc. but it would still have worked in the browser. What is important here (the actual fix) is that for packages to consume the local code, they need to reference a specific version that matches the local version without ^ or ~ and so on, the version needs to be exactly the same. It also does not work by requesting "latest", etc.

@mtrutledge @donker @bdukes if you want to re-approve or merge, thanks.

[donker]

Is good. Working with Daniel on this.