Skip to content

Move file error message into redux#47137

Merged
sanchitmalhotra126 merged 1 commit intosanchit/javalab-editor-refactor/split-dialog-headerfrom
sanchit/javalab-editor-refactor/file-errors-redux
Jul 28, 2022
Merged

Move file error message into redux#47137
sanchitmalhotra126 merged 1 commit intosanchit/javalab-editor-refactor/split-dialog-headerfrom
sanchit/javalab-editor-refactor/file-errors-redux

Conversation

@sanchitmalhotra126
Copy link
Copy Markdown
Contributor

Quick follow-up to #47116. This moves the new/renameFileError values into redux to reduce state coupling between JavalabEditor and JavalabEditorDialogManager. As with the previous PRs, this is branched off of the previous refactor PR and we plan to keep these unmerged until all the works has been completed and reviewed to avoid unnecessarily rewriting unit tests.

Links

Testing story

Tested locally.

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@sanchitmalhotra126 sanchitmalhotra126 requested a review from a team July 8, 2022 00:35
type: CLEAR_NEW_FILE_ERROR
});

export const setRenameFileError = error => ({
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is there a reason for having both set... and clear... functions? Could we just call set... with null?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Just for clarity - we could also call set... with null and it would do the same thing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we simplify to just have set... then? Mainly to reduce the number of things we have to pass around/import

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, my inclination to do it this way was just that clearError seems a little more encapsulated than setError(null) because it lets the redux file handle what "clear" means. Just in case something like setError(undefined) yields unexpected results because we're expecting null somewhere else. I don't have a strong preference though so I can simplify this to reduce the number of props.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@sanchitmalhotra126 I think you are on the right track that the action describes the event (a error was cleared) and the reducer determines how that impacts the state (sets something to null).

@lindapaiste
Copy link
Copy Markdown

I'm just looking at this codebase for the first time, but have y'all considered using the official Redux Toolkit for some of your reducers? It is so much easier to write, especially when there are loads of action constants like here. With createSlice you just define the reducer case for each action and it generates the action type strings and action creator functions automatically.

If this is something that you are interested in, I'd be happy to help!

I haven't converted the whole file, but here is part of it to give you an idea:

const { reducer, actions } = createSlice({
    // this becomes the prefix for all action type names
    name: 'javalab',
    // same initialState as before
    initialState,
    // define the handling for each action, using mutations on a draft state object.
    reducers: {
        appendInputLog: (state, action) => {
            state.consoleLogs.push({ type: 'input', text: action.payload });
        },
        appendOutputLog: (state, action) => {
            state.consoleLogs.push({ type: 'output', text: action.payload });
        },
        appendNewlineToConsoleLog: (state) => {
            state.consoleLogs.push({ type: 'newline' });
        },
        appendMarkdownLog: (state, action) => {
            state.consoleLogs.push({ type: 'markdown', text: action.payload });
        },
        clearConsoleLogs: (state) => {
            state.consoleLogs = [];
        },
        setRenderedHeight: (state, action) => {
            state.renderedEditorHeight = action.payload;
        },
        setLeftWidth: (state, action) => {
            state.leftWidth = action.payload;
        },
        setRightWidth: (state, action) => {
            state.rightWidth = action.payload;
        },
        setInstructionsHeight: (state, action) => {
            state.instructionsHeight = action.payload;
        },
        setInstructionsFullHeight: (state, action) => {
            state.instructionsFullHeight = action.payload;
        },
        setConsoleHeight: (state, action) => {
            state.consoleHeight = action.payload;
        },
        setEditorColumnHeight: (state, action) => {
            state.editorColumnHeight = action.payload;
        }
    }
});

export default reducer;

// action creator functions are automatically generated!
export const {
    appendInputLog,
    appendOutputLog,
    appendNewlineToConsoleLog,
    appendMarkdownLog,
    clearConsoleLogs,
    setRenderedHeight,
    setLeftWidth,
    setRightWidth,
    setInstructionsHeight,
    setInstructionsFullHeight,
    setConsoleHeight,
    setEditorColumnHeight
} = actions;

Comment on lines +134 to +138
dispatch => ({
closeEditorDialog: () => dispatch(closeEditorDialog()),
clearNewFileError: () => dispatch(clearNewFileError()),
clearRenameFileError: () => dispatch(clearRenameFileError())
})
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You can use the use the object syntax of mapDispatchToProps. These 5 lines can be replaced with:

{ closeEditorDialog, clearNewFileError, clearRenameFileError }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm just looking at this codebase for the first time, but have y'all considered using the official Redux Toolkit for some of your reducers? It is so much easier to write, especially when there are loads of action constants like here. With createSlice you just define the reducer case for each action and it generates the action type strings and action creator functions automatically.

If this is something that you are interested in, I'd be happy to help!

I haven't converted the whole file, but here is part of it to give you an idea:

const { reducer, actions } = createSlice({
    // this becomes the prefix for all action type names
    name: 'javalab',
    // same initialState as before
    initialState,
    // define the handling for each action, using mutations on a draft state object.
    reducers: {
        appendInputLog: (state, action) => {
            state.consoleLogs.push({ type: 'input', text: action.payload });
        },
        appendOutputLog: (state, action) => {
            state.consoleLogs.push({ type: 'output', text: action.payload });
        },
        appendNewlineToConsoleLog: (state) => {
            state.consoleLogs.push({ type: 'newline' });
        },
        appendMarkdownLog: (state, action) => {
            state.consoleLogs.push({ type: 'markdown', text: action.payload });
        },
        clearConsoleLogs: (state) => {
            state.consoleLogs = [];
        },
        setRenderedHeight: (state, action) => {
            state.renderedEditorHeight = action.payload;
        },
        setLeftWidth: (state, action) => {
            state.leftWidth = action.payload;
        },
        setRightWidth: (state, action) => {
            state.rightWidth = action.payload;
        },
        setInstructionsHeight: (state, action) => {
            state.instructionsHeight = action.payload;
        },
        setInstructionsFullHeight: (state, action) => {
            state.instructionsFullHeight = action.payload;
        },
        setConsoleHeight: (state, action) => {
            state.consoleHeight = action.payload;
        },
        setEditorColumnHeight: (state, action) => {
            state.editorColumnHeight = action.payload;
        }
    }
});

export default reducer;

// action creator functions are automatically generated!
export const {
    appendInputLog,
    appendOutputLog,
    appendNewlineToConsoleLog,
    appendMarkdownLog,
    clearConsoleLogs,
    setRenderedHeight,
    setLeftWidth,
    setRightWidth,
    setInstructionsHeight,
    setInstructionsFullHeight,
    setConsoleHeight,
    setEditorColumnHeight
} = actions;

Hi @lindapaiste thanks for the suggestion! I believe use a slightly customized version of redux but this is definitely something we can look into.

@sanchitmalhotra126 sanchitmalhotra126 merged commit c00b609 into sanchit/javalab-editor-refactor/split-dialog-header Jul 28, 2022
@sanchitmalhotra126 sanchitmalhotra126 deleted the sanchit/javalab-editor-refactor/file-errors-redux branch July 28, 2022 17:09
@sanchitmalhotra126 sanchitmalhotra126 restored the sanchit/javalab-editor-refactor/file-errors-redux branch July 29, 2022 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants