-
Notifications
You must be signed in to change notification settings - Fork 71
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
import named export from parcel/watcher #2341
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 4f0c274 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
// eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
const subscribeMock = mock.fn(async (_dir, _effect, _options) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// eslint-disable-next-line @typescript-eslint/no-unused-vars | |
const subscribeMock = mock.fn(async (_dir, _effect, _options) => { | |
const subscribeMock = mock.fn(async () => { |
that should be fine, if not then something like this:
amplify-backend/packages/client-config/src/client-config-writer/client_config_writer.test.ts
Line 27 in 1400e6f
writeFile: mock.fn<(path: string, content: string) => Promise<void>>(() => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was to appease the checks like this one where we're checking that subscribe was called with a specific argument, https://github.com/aws-amplify/amplify-backend/pull/2341/files#diff-a3a9ef2ca41c1c9b876d1523afccb0fa5478ffd7ef185395e103467e10bd6484R967
I can pull in the parameters type to give the arguments a type. The underscore satisfies tsc but I also had to add the eslint-disable comment to appease no-unused-vars
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps
const subscribeMock = mock.fn(async (..._args: Parameters<typeof subscribe>) => {
return { unsubscribe: unsubscribeMockFn };
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the mock.fn<>
satisfies both tsc and linter. Let's define parameterless stub AND provides typing for argument assertion.
The _dir
or ..._args
are not relevant for mock
. this is for TSC - but that can be handled with <>
Problem
follow-up from #2332
in a limited env like WebContainers we're seeing issues related to the import of the "default" export from the commonjs distribution of parcel/watcher. "default" exports are a concept in ESM, where commonjs will export on
module.exports
orexports
. Depending on the build process these are sometimes rebound to a nameddefault
->module.exports.default = "something"
or merged ontomodule.exports
Issue number, if available:
Changes
This change removes the import of parcel/watchers default export in favor of the named
subscribe
export.subscribe
was already in use for typing, so this change is smallCorresponding docs PR, if applicable:
Validation
validated by re-running the sandbox command in the limited WebContainers env
Checklist
run-e2e
label set.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.