This repository was archived by the owner on Jan 11, 2023. It is now read-only.
map selected source when we toggle pretty print#3285
Merged
codehag merged 9 commits intoAug 3, 2017
Conversation
b822558 to
766b46e
Compare
Codecov Report
@@ Coverage Diff @@
## master #3285 +/- ##
=========================================
- Coverage 49.35% 49.3% -0.05%
=========================================
Files 99 99
Lines 4156 4160 +4
Branches 856 857 +1
=========================================
Hits 2051 2051
- Misses 2105 2109 +4
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #3285 +/- ##
=========================================
+ Coverage 51.97% 52.3% +0.32%
=========================================
Files 110 112 +2
Lines 4529 4562 +33
Branches 936 940 +4
=========================================
+ Hits 2354 2386 +32
- Misses 2175 2176 +1
Continue to review full report at Codecov.
|
codehag
reviewed
Jul 10, 2017
Contributor
codehag
left a comment
There was a problem hiding this comment.
Looks good! maybe we can write a test for this flow?
Contributor
|
Mochi test needs an update |
Contributor
|
I will take a look at getting that mochitest up and running / add a test! |
Contributor
|
Not too sure what is going on... I have spent a few hours on it. But it looks like the highlighting of the line in pretty print is not working when you run the test. Maybe its worth another look? Full output of the error: |
Contributor
|
So I have narrowed down why this is failing:
Another issue I noticed: if the PP file is closed there is no way to reopen it. I will look into these two for a little bit and see if i can resolve these two issues. |
2961e56 to
7a2769b
Compare
codehag
reviewed
Jul 31, 2017
| case "ADD_PRETTY_SOURCE": { | ||
| const { source, generatedSource, mappings } = action; | ||
|
|
||
| function updateLocation({ location }) { |
Contributor
There was a problem hiding this comment.
maybe we can move this out of the reducer, wanted a bit of feedback on it though if it would be too slow
ca0b78a to
05d0e08
Compare
codehag
reviewed
Aug 2, 2017
| */ | ||
| export function selectSource(id: string, options: SelectSourceOptions = {}) { | ||
| return async ({ dispatch, getState, client }: ThunkArgs) => { | ||
| return ({ dispatch, getState, sourceMaps, client }: ThunkArgs) => { |
Contributor
There was a problem hiding this comment.
ah we don't need sourcemaps here
codehag
reviewed
Aug 2, 2017
|
|
||
| clickElement(dbg, "prettyPrintButton"); | ||
| yield waitForDispatch(dbg, "TOGGLE_PRETTY_PRINT"); | ||
| debugger; |
jasonLaster
commented
Aug 2, 2017
| }; | ||
| } | ||
|
|
||
| function remapLocations(breakpoints, sourceId, sourceMaps) { |
Contributor
Author
There was a problem hiding this comment.
does async function work?
If so, can we add an await here. I'm guessing no because I bet the Promise.all was a hack lol
Contributor
There was a problem hiding this comment.
The promise api is still useful in async await, and promise.all is the suggested way to resolve a group of promises. i never had to use it before, and it was a stumbling block. Async/Await can resolve single promises, but not a collection. infos --> tc39/proposal-async-await#61
| sourceMaps | ||
| ); | ||
|
|
||
| return dispatch({ |
Contributor
Author
There was a problem hiding this comment.
this reads really well!
jasonLaster
commented
Aug 2, 2017
Contributor
Author
jasonLaster
left a comment
There was a problem hiding this comment.
Looking great
| }; | ||
| } | ||
|
|
||
| export function remapBreakpoints(sourceId: string) { |
Contributor
Author
There was a problem hiding this comment.
what do you think of moving remapBreakpoints to actions/breakpoints/remapBreakpoints?
| frames = await updateFrameLocations(frames, sourceMaps); | ||
| } | ||
| if (!prettySource) { | ||
| await dispatch(createPrettySource(sourceId)); |
Contributor
Author
There was a problem hiding this comment.
could we do:
const prettySource = await dispatch(createPrettySource(sourceId));| expect(pretty.get("contentType")).toEqual("text/javascript"); | ||
| expect(pretty.get("url").includes(prettyURL)).toEqual(true); | ||
| expect(pretty).toMatchSnapshot(); | ||
| }); |
Contributor
Author
There was a problem hiding this comment.
how about we add two more tests
- also has breakpoints mapped
- also has selected location mapped
Contributor
There was a problem hiding this comment.
I didnt have time to do this, and there was an unusual error so I made a ticket to tackle it later: #3531
|
|
||
| .theme-dark .symbol-modal { | ||
| box-shadow: 2px 4px 6px var(--popup-shadow-color); | ||
| } |
Contributor
Author
There was a problem hiding this comment.
it's now in modal.css
codehag
reviewed
Aug 2, 2017
| return state.setIn(["breakpoints", locationId], breakpoint); | ||
| } | ||
|
|
||
| function remapBreakpoint(state, action) { |
Contributor
There was a problem hiding this comment.
remapBreakpoint -> remapBreakpoints
77a6deb to
87ef289
Compare
87ef289 to
ec2d078
Compare
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
Associated Issue: #2883
Summary of Changes
Test Plan
updating the mochitest now