-
Notifications
You must be signed in to change notification settings - Fork 754
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
Fixes dnn-react-common build #3462
Merged
Merged
Changes from 15 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
2e2ffa5
Should fix common bundle build
valadas 1aacfe1
One more little fix
valadas 2de35d1
One more try
valadas 6280106
Update all package.json first
valadas c90f29c
Add react-common to glob
valadas 8ed366b
Missing ;
valadas a4a4369
eslint ignore dist files
valadas 4784168
Remove version scripts on common
valadas dcb84d8
Trying Matt latest info
valadas 697820d
Removed back eslint from workspace root
valadas 7a7ddf7
Remove parallel build of clientside
valadas 3589ac3
Replacing parallel for stream
valadas b247978
Fix svg icons
valadas 3cd01b3
Revert test change
valadas 32861e2
Commit package.json and yarn.lock now that it works
valadas 724d249
Merged changes after #3456 got merged
valadas 61335d3
Re-comming package.json versions
valadas 6f23589
Fixed whitespace and uneeded diffs
valadas File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
src/components/contentTypeModal/fieldDefinitions/FileUpload/Dropzone.jsx | ||
/src/vendor/** | ||
/src/utils/masker.js | ||
/src/utils/masker.js | ||
dnn-react-common.min.js |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
164 changes: 82 additions & 82 deletions
164
Dnn.AdminExperience/ClientSide/Dnn.React.Common/src/SvgIcons/index.jsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@valadas I don't think @mtrutledge is arguing against switching to
--stream
, he's just saying the command should be in thepackage.json
instead of this MSBuild file. See https://github.com/dnnsoftware/Dnn.Platform/pull/3456/files#diff-b9cfc7f2cdf78a7f4b91a753d10865a2R30-R36, where thescripts
section is added topackage.json
, so that the MSBuild file can just callyarn build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes @bdukes and @valadas that is what I was saying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah sure. Wont be able to submit until late or tomorrow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.