Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Improve JSX syntax highlighting #4539

Merged
26 changes: 25 additions & 1 deletion src/actions/ast.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ import { PROMISE } from "../utils/redux/middleware/promise";
import {
getSymbols,
getEmptyLines,
getOutOfScopeLocations
getOutOfScopeLocations,
isReactComponent
} from "../workers/parser";

import { findBestMatchExpression } from "../utils/ast";
Expand All @@ -29,6 +30,29 @@ const extraProps = {
react: { displayName: "this._reactInternalInstance.getName()" }
};

export function setMetaData(sourceId: SourceId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

lets add a jest unit test for this next to setSymbols

return async ({ dispatch, getState }: ThunkArgs) => {
const sourceRecord = getSource(getState(), sourceId);
if (!sourceRecord) {
return;
}

const source = sourceRecord.toJS();
if (!source.text || source.isWasm) {
return;
}

const isReactComp = await isReactComponent(source);
dispatch({
type: "SET_SOURCE_METADATA",
source,
metaData: {
isReactComponent: isReactComp
}
});
};
}

export function setSymbols(sourceId: SourceId) {
return async ({ dispatch, getState }: ThunkArgs) => {
const sourceRecord = getSource(getState(), sourceId);
Expand Down
3 changes: 2 additions & 1 deletion src/actions/sources/loadSourceText.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// @flow
import { PROMISE } from "../../utils/redux/middleware/promise";
import { setEmptyLines, setSymbols } from "../ast";
import { setEmptyLines, setSymbols, setMetaData } from "../ast";
import { getSource } from "../../selectors";
import { setSource } from "../../workers/parser";
import type { Source } from "../../types";
Expand Down Expand Up @@ -45,5 +45,6 @@ export function loadSourceText(source: Source) {
await setSource(newSource);
await dispatch(setSymbols(source.id));
await dispatch(setEmptyLines(source.id));
await dispatch(setMetaData(source.id));
};
}
7 changes: 4 additions & 3 deletions src/components/Editor/Breakpoint.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ function makeMarker(isDisabled: boolean) {
type Props = {
breakpoint: Object,
selectedSource: Object,
editor: Object
editor: Object,
metaData: Object
};

class Breakpoint extends Component<Props> {
Expand All @@ -36,7 +37,7 @@ class Breakpoint extends Component<Props> {
}

addBreakpoint() {
const { breakpoint, editor, selectedSource } = this.props;
const { breakpoint, editor, selectedSource, metaData } = this.props;

// Hidden Breakpoints are never rendered on the client
if (breakpoint.hidden) {
Expand All @@ -52,7 +53,7 @@ class Breakpoint extends Component<Props> {
const sourceId = selectedSource.get("id");
const line = toEditorLine(sourceId, breakpoint.location.line);

showSourceText(editor, selectedSource.toJS());
showSourceText(editor, selectedSource.toJS(), metaData);
Copy link
Contributor

Choose a reason for hiding this comment

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

@codehag do we need showSourceText after the lifecycle work?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure why this is in addBreakpoint to be honest. this shouldnt be taken care of here -- it should be done with the lifecycle update... in fact the same can be said about everything that is happening with the editor.codemirror here. this is outside of the scope of this pr... good catch though

Copy link
Contributor

@codehag codehag Nov 3, 2017

Choose a reason for hiding this comment

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

ahh ok i understand better. the naming is a bit off -- this should probably be renderBreakpoint; still not sure if we need showSourceText... this was added here: #3294

Copy link
Contributor

Choose a reason for hiding this comment

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

Filed #4579 and we can come back


editor.codeMirror.setGutterMarker(
line,
Expand Down
11 changes: 7 additions & 4 deletions src/components/Editor/Breakpoints.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import React, { Component } from "react";
import Breakpoint from "./Breakpoint";

import actions from "../../actions";
import { getSelectedSource } from "../../selectors";
import { getSelectedSource, getMetaData } from "../../selectors";
import getVisibleBreakpoints from "../../selectors/visibleBreakpoints";
import { makeLocationId } from "../../utils/breakpoint";
import { isLoaded } from "../../utils/source";
Expand All @@ -16,7 +16,8 @@ import type { SourceRecord, BreakpointsMap } from "../../reducers/types";
type Props = {
selectedSource: SourceRecord,
breakpoints: BreakpointsMap,
editor: Object
editor: Object,
metaData: Object
};

class Breakpoints extends Component<Props> {
Expand All @@ -32,7 +33,7 @@ class Breakpoints extends Component<Props> {
}

render() {
const { breakpoints, selectedSource, editor } = this.props;
const { breakpoints, selectedSource, editor, metaData } = this.props;

if (!selectedSource || !breakpoints || selectedSource.get("isBlackBoxed")) {
return null;
Expand All @@ -46,6 +47,7 @@ class Breakpoints extends Component<Props> {
key={makeLocationId(bp.location)}
breakpoint={bp}
selectedSource={selectedSource}
metaData={metaData}
editor={editor}
/>
);
Expand All @@ -58,7 +60,8 @@ class Breakpoints extends Component<Props> {
export default connect(
state => ({
breakpoints: getVisibleBreakpoints(state),
selectedSource: getSelectedSource(state)
selectedSource: getSelectedSource(state),
metaData: getMetaData(state)
}),
dispatch => bindActionCreators(actions, dispatch)
)(Breakpoints);
10 changes: 7 additions & 3 deletions src/components/Editor/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
getSelectedSource,
getHitCountForSource,
getCoverageEnabled,
getMetaData,
getConditionalPanelLine
} from "../../selectors";

Expand All @@ -35,6 +36,7 @@ import EmptyLines from "./EmptyLines";
import GutterMenu from "./GutterMenu";
import EditorMenu from "./EditorMenu";
import ConditionalPanel from "./ConditionalPanel";
import type { SourceMetaDataType } from "../../reducers/ast";

import {
showSourceText,
Expand Down Expand Up @@ -74,6 +76,7 @@ type Props = {
startPanelSize: number,
endPanelSize: number,
conditionalPanelLine: number,
metaData: SourceMetaDataType,

// Actions
openConditionalPanel: number => void,
Expand Down Expand Up @@ -453,7 +456,7 @@ class Editor extends PureComponent<Props, State> {
}

setText(props) {
const { selectedSource } = props;
const { selectedSource, metaData } = props;
if (!this.state.editor) {
return;
}
Expand All @@ -471,7 +474,7 @@ class Editor extends PureComponent<Props, State> {
}

if (selectedSource) {
return showSourceText(this.state.editor, selectedSource.toJS());
return showSourceText(this.state.editor, selectedSource.toJS(), metaData);
}
}

Expand Down Expand Up @@ -597,7 +600,8 @@ const mapStateToProps = state => {
hitCount: getHitCountForSource(state, sourceId),
selectedFrame: getSelectedFrame(state),
coverageOn: getCoverageEnabled(state),
conditionalPanelLine: getConditionalPanelLine(state)
conditionalPanelLine: getConditionalPanelLine(state),
metaData: getMetaData(state)
};
};

Expand Down
20 changes: 18 additions & 2 deletions src/reducers/ast.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,12 @@ type EmptyLinesType = number[];
export type SymbolsMap = Map<string, SymbolDeclarations>;
export type EmptyLinesMap = Map<string, EmptyLinesType>;

export type SourceMetaDataType = {
isReactComponent: boolean
};

export type SourceMetaDataMap = Map<string, SourceMetaDataType>;

export type Preview =
| {| updating: true |}
| null
Expand All @@ -38,7 +44,8 @@ export type ASTState = {
symbols: SymbolsMap,
emptyLines: EmptyLinesMap,
outOfScopeLocations: ?Array<AstLocation>,
preview: Preview
preview: Preview,
metaData: SourceMetaDataMap
};

export function initialState() {
Expand All @@ -47,7 +54,8 @@ export function initialState() {
symbols: I.Map(),
emptyLines: I.Map(),
outOfScopeLocations: null,
preview: null
preview: null,
metaData: I.Map()
}: ASTState)
)();
}
Expand Down Expand Up @@ -111,6 +119,10 @@ function update(
return initialState();
}

case "SET_SOURCE_METADATA": {
return state.set("metaData", action.metaData);
Copy link
Contributor

Choose a reason for hiding this comment

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

ahh, I'd change this to look like empty lines: return state.setIn(["emptyLines", source.id], emptyLines);

}

default: {
return state;
}
Expand Down Expand Up @@ -167,4 +179,8 @@ export function getPreview(state: OuterState) {
return state.ast.get("preview");
}

export function getMetaData(state: OuterState) {
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, i think this should be getSourceMetaData(state: OuterState, source: Source) so that we scope it down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do we need source for in this selector?

return state.ast.get("metaData");
}

export default update;
11 changes: 9 additions & 2 deletions src/utils/editor/source-documents.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { getMode } from "../source";
import type { Source } from "debugger-html";
import { isWasm, getWasmLineNumberFormatter, renderWasmText } from "../wasm";
import { resizeBreakpointGutter, resizeToggleButton } from "../ui";
import type { SourceMetaDataType } from "../../reducers/ast";

let sourceDocs = {};

Expand Down Expand Up @@ -81,19 +82,25 @@ function setEditorText(editor: Object, source: Source) {
* Handle getting the source document or creating a new
* document with the correct mode and text.
*/
function showSourceText(editor: Object, source: Source) {
function showSourceText(
editor: Object,
source: Source,
metaData: SourceMetaDataType
) {
if (!source) {
return;
}

let doc = getDocument(source.id);
if (editor.codeMirror.doc === doc) {
editor.setMode(getMode(source, metaData));
return;
}

if (doc) {
editor.replaceDocument(doc);
updateLineNumberFormat(editor, source.id);
editor.setMode(getMode(source, metaData));
return doc;
}

Expand All @@ -102,7 +109,7 @@ function showSourceText(editor: Object, source: Source) {
editor.replaceDocument(doc);

setEditorText(editor, source);
editor.setMode(getMode(source));
editor.setMode(getMode(source, metaData));
updateLineNumberFormat(editor, source.id);
}

Expand Down
7 changes: 6 additions & 1 deletion src/utils/source.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { basename } from "../utils/path";
import { parse as parseURL } from "url";

import type { Source } from "../types";
import type { SourceMetaDataType } from "../reducers/ast";

type transformUrlCallback = string => string;

Expand Down Expand Up @@ -204,13 +205,17 @@ function getSourceLineCount(source: Source) {
* @static
*/

function getMode(source: Source) {
function getMode(source: Source, metaData: SourceMetaDataType) {
const { contentType, text, isWasm, url } = source;

if (!text || isWasm) {
return { name: "text" };
}

if (metaData && metaData.isReactComponent) {
return { name: "jsx" };
}

// if the url ends with .marko we set the name to Javascript so
// syntax highlighting works for marko too
if (url && url.match(/\.marko$/i)) {
Expand Down
5 changes: 4 additions & 1 deletion src/workers/parser/frameworks.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,10 @@ function importsReact(imports) {
function extendsComponent(classes) {
let result = false;
classes.some(classObj => {
if (classObj.parent.name === "Component") {
if (
classObj.parent.name === "Component" ||
classObj.parent.name === "PureComponent"
) {
result = true;
}
});
Expand Down
1 change: 1 addition & 0 deletions src/workers/parser/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export const hasSource = dispatcher.task("hasSource");
export const setSource = dispatcher.task("setSource");
export const clearSources = dispatcher.task("clearSources");
export const hasSyntaxError = dispatcher.task("hasSyntaxError");
export const isReactComponent = dispatcher.task("isReactComponent");

export type { SymbolDeclaration, SymbolDeclarations } from "./getSymbols";
export type { AstLocation } from "./types";
4 changes: 3 additions & 1 deletion src/workers/parser/worker.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import getOutOfScopeLocations from "./getOutOfScopeLocations";
import { getNextStep } from "./steps";
import getEmptyLines from "./getEmptyLines";
import { hasSyntaxError } from "./validate";
import { isReactComponent } from "./frameworks";

import { workerUtils } from "devtools-utils";
const { workerHandler } = workerUtils;
Expand All @@ -26,5 +27,6 @@ self.onmessage = workerHandler({
getVariablesInScope,
getNextStep,
getEmptyLines,
hasSyntaxError
hasSyntaxError,
isReactComponent
});