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

draw function snapshot testing #15

Merged
merged 4 commits into from
Jan 7, 2024

Conversation

TheTallJerry
Copy link
Collaborator

@TheTallJerry TheTallJerry marked this pull request as ready for review January 5, 2024 20:35
.husky/pre-commit Outdated Show resolved Hide resolved
Copy link
Owner

@david-yz-liu david-yz-liu left a comment

Choose a reason for hiding this comment

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

Great work, @TheTallJerry!

package.json Outdated Show resolved Hide resolved
src/memory_model.ts Show resolved Hide resolved
src/style.ts Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
@@ -0,0 +1,34 @@
import { MemoryModel } from "../src/memory_model";
Copy link
Owner

Choose a reason for hiding this comment

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

The two imports are inconsistent, and they shouldn't need to be. You can import both MemoryModel and draw from ../src/index, as that file exports both of them. (Not sure if importing from index.js is special in some way.)

Copy link
Collaborator Author

@TheTallJerry TheTallJerry Jan 7, 2024

Choose a reason for hiding this comment

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

This evolved into something larger. Essentially, the new import now looks like

import exports from "../src";
const { MemoryModel, draw } = exports;

which is valid syntax (and cleaner) outside of this context. However, it appears that RoughJS relies on ESM exports, which Jest has confirmed that it won't support. This comes into play with the draw function as it eventually calls RoughJS.

As a result, with this setup, Jest reports an error along the lines of Unexpected token export. This is fixed by forcing a manual resolve with Jest, as mentioned on stackoverflow and specifically this comment. The updated Jest config now has

  moduleNameMapper: {
    // Force module roughjs to resolve with the CJS entry point, because Jest does not support package.json.exports. Elaborated in PR#15.
    "roughjs": require.resolve('roughjs'),
  },

If Jest at some point supports ESM, or if RoughJS gets updated to be compatible with Jest in this aspect, then this configuration can be removed. As shown above, I left a comment alongside the config for future reference.

Copy link
Collaborator Author

@TheTallJerry TheTallJerry Jan 7, 2024

Choose a reason for hiding this comment

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

Related: because MemoryModel is exported as a class and then dynamically imported, in order to use it as a type, I had to use InstanceType<typeof MemoryModel> instead of simply MemoryModel. See this stackoverflow comment for more elaboration.

tests/draw.spec.ts Outdated Show resolved Hide resolved
src/memory_model.ts Outdated Show resolved Hide resolved
Copy link
Owner

@david-yz-liu david-yz-liu left a comment

Choose a reason for hiding this comment

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

Great work, thank you @TheTallJerry!

@david-yz-liu david-yz-liu merged commit 837bf0c into david-yz-liu:main Jan 7, 2024
@TheTallJerry TheTallJerry deleted the draw-snapshot-testing branch January 8, 2024 04:03
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.

2 participants