Skip to content
This repository has been archived by the owner on Oct 6, 2019. It is now read-only.

Uses source-map for webpack config #797

Merged
merged 4 commits into from
Apr 16, 2019

Conversation

valadas
Copy link
Contributor

@valadas valadas commented Apr 14, 2019

Closes #419

Summary

Source maps are js files that help debug minified javascript by mapping the minified version to the original source code that generated them before transpiling. Webpack allows generating those files in multiple ways.

Some modules had the inline-source-map option which includes the source code inside the bundle file and makes it huge for no good reason. For those, this PR will reduce their footprint without loosing the features.

Some other modules had no source maps to begin with, this PR will make the cryptic error message more meaningful and help use pinpoint strange hard to reproduce issues and edge cases more easily. Note that the source map files are only loaded by the browser development tools, they do not affect normal site users in production.

@valadas valadas added this to the 2.0.1 milestone Apr 14, 2019
Copy link
Contributor

@ohine ohine left a comment

Choose a reason for hiding this comment

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

This looks great! One minor tweak, can you easily fix the white space differences in the solution file? I’m worried about it conflicting with #693

@valadas
Copy link
Contributor Author

valadas commented Apr 15, 2019

I am not sure what you mean @ohine if I view it (without ignore whitespace) I see no problem? Am I looking at it wrong, where do you see whitespace issues ?

donker
donker previously approved these changes Apr 16, 2019
Copy link
Contributor

@bdukes bdukes left a comment

Choose a reason for hiding this comment

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

I reverted the whitespace changes to the sln file (I saw it in some views but not others, I think the tools are just trying to be too helpful)

@bdukes bdukes requested a review from ohine April 16, 2019 21:04
@david-poindexter david-poindexter merged commit cc16706 into dnnsoftware:release/2.0.x Apr 16, 2019
@valadas
Copy link
Contributor Author

valadas commented Apr 17, 2019

Thanks @bdukes can you explain me how you fixed this.

How you push a commit to someone else PR and how you revert a whole file commit ? Just want to learn this.

@bdukes
Copy link
Contributor

bdukes commented Apr 17, 2019

When you create a PR, there's a box checked by default, "Allow edits from maintainers"
image

So I was able to pull down your branch, add a commit, and push it back up to your branch.

For the whitespace change, I reverted your change to the sln file, and then used the Commit View in Git Extensions and (with the whitespace filter on so I only saw non-whitespace changes) reset the lines with substantial changes (that is, undid the revert, to leave them as they were). What was then left was just the whitespace changes being reverted, so I committed what was left.

Does that help?

@valadas
Copy link
Contributor Author

valadas commented Apr 19, 2019

That is kinda how I tried to do this in the past, but it never worked for me, not sure why. Maybe one day we can look at this together, see what I am doing wrong...

@ohine
Copy link
Contributor

ohine commented Apr 19, 2019

This is how I've done it successfully when working with #717

git clone https://github.com/donker/Dnn.AdminExperience.git .
git remote add upstream https://github.com/dnnsoftware/AdminExperience.git
git fetch upstream
git checkout gdprv2
git merge upstream/development

Open Visual Studio, update references, build.

git add ....
git commit -m "my message"
git push

I know @bdukes has a different workflow, which I wasn't able to get to work right (forgot the exact error). I'd love to figure out one of these days what I did wrong because his method seems much quicker/cleaner as it doesn't require cloning an entire fork I believe.

@valadas
Copy link
Contributor Author

valadas commented Apr 19, 2019

Hmm, interesting. I think github instructions confused me, your way makes sense, you are actually allowed to push to the submitter branch then. I was trying to push to upstream or something... I will try that next time. Thanks.

@bdukes
Copy link
Contributor

bdukes commented Apr 19, 2019

Here are the commands I used:

git checkout release/2.0.x
git checkout -b valadas_issue-419 
git pull https://github.com/valadas/Dnn.AdminExperience issue-419
gitex
git push https://github.com/valadas/Dnn.AdminExperience valadas_issue-419:issue-419

I did not have a local copy of the release/2.0.x branch yet, so the first checkout created a local branch from origin/release/2.0.x. Otherwise, I would have needed to fetch and reset to make sure I was on the latest version of the destination branch.

gitex opened Git Extensions for the repo, and that's where I did the work to revert the sln file changes.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants