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

Menus and React Testing Library migration #359

Merged
merged 38 commits into from
Mar 13, 2021
Merged

Conversation

nelsonni
Copy link
Member

@nelsonni nelsonni commented Mar 6, 2021

Description:

The initial interaction points within the Synectic canvas have been based on buttons managed within their respective card, stack, error, and dialog components. This is not a sustainable architecture going forward. A unified navigation menu, similar to the design considerations of https://material.io/components/menus, has prompted a refactoring to a single NavMenu component that launches all interactions via Redux actions.

Additionally, we have been maintaining tests written for both Enzyme and React Testing Library (RTL). However, issues such as #357 highlight that this structure is untenable as dependencies for their testing libraries begin to conflict. Its time to cut-over to using React Testing Library (RTL) only, and any remaining Enzyme tests need to be updated.

This PR resolves #90, #357, and #113, and signifies the following version changes:

  • MAJOR version increase
  • MINOR version increase
  • PATCH version increase

Changes:

This PR makes the following changes:

Checklist:

Before submitting this PR, I have verified that my code:

  • Resides on a fix/ or feature/ branch that was initially branched off from development.
  • Passes code linting (yarn lint) and unit testing (yarn test).
  • Successfully builds a distribution package (yarn package).

Additionally, I have verified that:

  • My name is listed in the Contributors section, or this PR includes a request to add my name.
  • I have read and am aware of the CONTRIBUTING guide for this project.

@nelsonni nelsonni added bug Bug reports or bug fixes feature Feature requests or improvements labels Mar 6, 2021
@nelsonni nelsonni added this to the v1.0.0 milestone Mar 6, 2021
@nelsonni nelsonni requested a review from jettseale March 6, 2021 21:50
Copy link
Collaborator

@jettseale jettseale left a comment

Choose a reason for hiding this comment

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

When I try to open a directory (File -> Open Directory...) it results in an error in the console and the file explorer card doesn't appear.

1

@nelsonni
Copy link
Member Author

nelsonni commented Mar 9, 2021

Analyzing this issue shows two likely separate problems on the Windows platform, that originate in the git.getStatus function:

  1. git.getRepoRoot returns . instead of undefined on empty directories. This function relies on calling isomorphic-git/findRoot to determine the correct path containing a .git directory or file, but the issue appears to be related to directories not under version control.
    export const getStatus = async (filepath: fs.PathLike): Promise<GitStatus | undefined> => {
    const repoRoot = await getRepoRoot(filepath);
    if (!repoRoot) return undefined; // no root Git directory indicates that the filepath is not part of a Git repo
  2. hidefile/shouldBeHiddenSync throws the Uncaught (in promise) TypeError: Cannot read property 'getAttributesSync' of undefined error when a directory path is passed to it.
    /** isomorphic-git provides `status()` for individual files, but requires `statusMatrix()` for directories
    * (per: https://github.com/isomorphic-git/isomorphic-git/issues/13) */
    if (await io.isDirectory(filepath)) {
    const statuses = await isogit.statusMatrix({ fs: fs, dir: dir, gitdir: gitdir, filter: f => !shouldBeHiddenSync(f) });
    const changed = statuses
    .filter(row => row[1] !== row[2]) // filter for files that have been changed since the last commit
    .map(row => row[0]); // return the filenames only
    return (changed.length > 0) ? 'modified' : 'unmodified';
    }

@nelsonni
Copy link
Member Author

nelsonni commented Mar 9, 2021

There is an open issue, stevenvachon/hidefile#3, that suggests others are also having trouble with the hidefile package on Windows. Added comment stevenvachon/hidefile#3 (comment) to that issue.

Fix in branch fix/windows-hidefiles should temporarily alleviate the issue by switching hidefile calls to use the underlying winattr package instead when the platform is Windows. @jettseale Please try this fix branch and see if it allows you to open directories again.

@nelsonni
Copy link
Member Author

nelsonni commented Mar 11, 2021

After testing the fix/windows-hidefiles branch with @jettseale, we determined that there are overlapping issues:

  1. hidefiles is failing to run on Windows platforms. So calls to hidefile/shouldBeHiddenSync and winattr/getSync both fail (hidefile depends on winattr, so the likely culprit is in winattr).
  2. The switch from useCallback to useEffect in the useDirectory custom React Hook (via b440fe6) is actually introducing a memory leak whenever a Explorer card is stacked and unstacked. The observed effect is that the contents of the Explorer card must reload, and when they do there is a chance that the following error will be thrown:
Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in a useEffect cleanup function.

@nelsonni
Copy link
Member Author

nelsonni commented Mar 11, 2021

The leaking memory error has actually been occurring on Windows for awhile now. After going back through the project history, we were able to narrow it down to being introduced sometime between c88e8cb (August 25, 2020) and e743c32 (September 11, 2020) on the development branch.

Also, it appears that the hidefile package was added during this development window as well (in commit 98817b1 on September 2, 2020).

@jettseale jettseale self-requested a review March 12, 2021 23:01
jettseale
jettseale previously approved these changes Mar 12, 2021
@nelsonni nelsonni merged commit 7685fee into development Mar 13, 2021
@nelsonni nelsonni deleted the feat/saving-changes branch March 13, 2021 00:16
@nelsonni nelsonni mentioned this pull request Jul 1, 2021
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug reports or bug fixes dependencies Issues or updates to dependency files feature Feature requests or improvements
Projects
None yet
2 participants