Skip to content
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

Implementing AST options #1

Closed
wants to merge 11 commits into from
Closed

Implementing AST options #1

wants to merge 11 commits into from

Conversation

benevbright
Copy link
Collaborator

No description provided.

@benevbright
Copy link
Collaborator Author

I created PR just to look the diff.

};

static getDerivedStateFromProps(nextProps: Props, prevState: State) {
const flattenSrc = flatten(nextProps.src);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No harmful to be inside if.

const flattenSrc = flatten(nextProps.src);
if (nextProps.src !== prevState.src) {
return {
src: unflatten(flattenSrc),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess it could be just src instead of using unflatten.

},
}));

this.onChangeJson(option);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I always prefer reducing the count of setState calls as much as I can.
So, it would be good if you combine this method's return value and setState above.

return null;
}

onOptionSettingCheck(option: string) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I suspect they prefer underscored name for member method.

this.onChangeJson(option);
}

onChangeJson(option: string) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I suspect they prefer underscored name for member method.

<div className={`${styles.panel} ${className}`}>
<div>
{OPTION_ORDER.map(option => {
return (
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You can reduce line since it has only return.

@@ -35,7 +35,7 @@ import {
import WorkerApi from "./WorkerApi";
import scopedEval from "./scopedEval";
import { colors, media } from "./styles";
import ReactJson from "react-json-view";
import ASTPanel from "./ASTPanel";

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

At compile method in Repl.js, their second parameter has astContext key, which should be added on Flow type as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't mean about above code example.
I wrote there because it was the only place I can leave a comment on Repl.js.

opts = opts || {};

const delimiter = opts.delimiter || ".";
const maxDepth = opts.maxDepth;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

need a default value.

src: Object,
flattenSrc: Object,
flattenType: Object,
astOption: {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

astOptions would be better name.

const { src, astOption, flattenSrc, flattenType } = this.state;
// const OPTION_ORDER = ["autofocus", "location", "empty", "type"];

function triggerAstOutput(type) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think option is a better name than type.

return result;
}

function deleteFlatten(currentSrc, deletedSrc) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IMO, I would not use deleteFlatten and mergeFlatten.
Instead, just filtering flattened src by options state at getDerivedStateFromProps.
Is there a reason using delete and merge? Is it cheaper?

Copy link
Collaborator Author

@benevbright benevbright left a comment

Choose a reason for hiding this comment

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

Awesome. I'm really impressed with how you're using JS skillfully.
To be honest, you're better than me 🤔

And this's just my opinion.
I sensed something is not natural first when I see the UI.
IMO, left sidebar is not a good place for AST checkout, because turning AST on makes disappear others. And I found this old PR. babel#1421
His approach was using the tab UI for AST on the top of the right panel. It makes sense to me.

And since Henry also mentioned below link that you guys will re-think UI. I guess you will figure it out. :)
babel#1736 (comment)

😴

[option]: !prevState.astOption[option],
},
},
this._onChangeJson(option)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's different with what I wanted to say.
There is no difference of count of setState call.

I meant that you could get a return value from _onChangeJson and setState together.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And there is also a chance this change is worse than the previous code.
(If React has an optimization of reducing the number of setState call under the hood)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants