[PrettyPrint] auto-pretty-print minified sources (#4171)#4700
Conversation
|
the commit also broke Editor.spec.js with to the following exception |
| isMinified(selected.id, selected.text) | ||
| ) { | ||
| togglePrettyPrint(selectedSource.get("id")); | ||
| } |
There was a problem hiding this comment.
can we do this in actions/sources#selectSource we like to have the actions control the business logic
| this.setText(nextProps); | ||
| if (nextProps.selectedSource !== this.props.selectedSource) { | ||
| this.setText(nextProps); | ||
| } |
There was a problem hiding this comment.
It was resetting the text each time, we setEmptylines and other stuff even though the selected source has not changed
…) - move logic to selectSource action
…) - move logic to selectSource action
|
Move the login to the action. |
|
Still some jest test are failing on my machine. |
|
kill the IF |
|
@assafdd yeah ..not relevant |
…) - fix expressions.spec.js test
|
@jasonLaster I removed the IF, fixed expressions.spec.js |
|
will look |
| await dispatch(loadSourceText(source.toJS())); | ||
| await dispatch(setOutOfScopeLocations()); | ||
| const src = getSource(getState(), id).toJS(); | ||
| if (src.text && isMinified(src.id, src.text)) { |
There was a problem hiding this comment.
@assafdd We should also be checking if this is a generated file or not before auto pretty printing. That's probably the issue with the tests aswell
There was a problem hiding this comment.
adding && !isOriginalId(src.id) should do the trick
…) - skip generated files
…) - a bit of linting
|
tiny lint issues |
|
Well, I think it's finally ready :) |
…`" (709:32) (firefox-devtools#4619) - bump pretty-fast & acorn to latest
…racter "`" (709:32) (firefox-devtools#4619) - bump pretty-fast & acorn to latest" This reverts commit b7cbbdd.
| await dispatch(loadSourceText(source.toJS())); | ||
| await dispatch(setOutOfScopeLocations()); | ||
| const src = getSource(getState(), id).toJS(); | ||
| if (src.text && !isOriginalId(src.id) && isMinified(src.id, src.text)) { |
There was a problem hiding this comment.
hey @assafdd,
just noticed one last tiny thing. we should do add shouldPrettyPrint(src). it'll look something like this
if (src.text && !isOriginalId(src.id) && (isMinified(src.id, src.text) || shouldPrettyPrint(src))) { so we make sure all holes are covered.
There was a problem hiding this comment.
could we do shouldPrettyPrint(src)) && isMinified(src.id, src.text)
There was a problem hiding this comment.
That might be too strict as there might be files that don't meet the isMinified heuristics but we might still want to pretty print if shouldPrettyPrint(src) is `true
There was a problem hiding this comment.
Yes we can. I move the empty text check to isMinified util.
Otherwise "should evaluate expressions in specific scope" in expressions.spec.js breaks.
There was a problem hiding this comment.
@bomsy I tried to play with shouldPrettyPrint() and its seems to be truthy on any JS source that I have checked.
There was a problem hiding this comment.
cool! it's fine, i had a scenario which i found but can't find it any more so its fine
|
Thanks! @assafdd |
Associated Issue: #4171
Summary of Changes
Test Plan:
run tests on
One odd is that if you close pretty-print source while it's selected it will reappear while if you close it while the original version is visible it won't.