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.

Sign up for GitHub

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

Jump to bottom

fix(commonjs): keep this context for callbacks #1603

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix(commonjs): keep this context for callbacks #1603

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
93387d2
keep this context for callbacks
patricklx Oct 5, 2023
f5cfc4d
Merge branch 'rollup:master' into patch-1
patricklx Oct 11, 2023
2d31405
missing parenthesis
patricklx Nov 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 4 additions & 4 deletions packages/commonjs/src/resolve-require-sources.js
View file
Open in desktop
Expand Up @@ -108,7 +108,7 @@ export function getRequireResolver(extensions, detectCyclesAndConditional, curre
setInitialParentType(parentId, parentMeta.initialCommonJSType);
await Promise.all(
parentRequires.map(({ resolved, isConditional }) =>
analyzeRequiredModule(parentId, resolved, isConditional, this.load)
analyzeRequiredModule(parentId, resolved, isConditional, this.load.bind(this))
Copy link
Member

Choose a reason for hiding this comment

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

Fair enough, but Rollup already this-binds all context functions. This is by design, but admittedly, this is not well-communicated.

Copy link
Author

@patricklx patricklx Oct 5, 2023 •
edited

Choose a reason for hiding this comment

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

ah, okay. this issue appeared while using commonjs plugin with vitejs.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting. So I assume this happened in dev mode? Then this would mean that Vite does not this.bind. Maybe an issue to raise with Vite. But this would also make it harder to write a test as you would need to test with a fake plugin context.

Copy link
Member

Choose a reason for hiding this comment

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

Also, Andrew will not like that you skipped the PR template where you should have added all this information.

Copy link
Member

Choose a reason for hiding this comment

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

I think so too ^^ 😅

Copy link
Author

Choose a reason for hiding this comment

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

I added it now

)
);
if (getTypeForFullyAnalyzedModule(parentId) !== parentMeta.isCommonJS) {
Expand Down Expand Up @@ -143,11 +143,11 @@ export function getRequireResolver(extensions, detectCyclesAndConditional, curre
(
await this.load({ id: resolved.id })
).meta.commonjs.resolved,
this.load
this.load.bind(this)
)) !== IS_WRAPPED_COMMONJS
);
}
return (await getTypeForImportedModule(resolved, this.load)) === IS_WRAPPED_COMMONJS;
return (await getTypeForImportedModule(resolved, this.load.bind(this))) === IS_WRAPPED_COMMONJS;
})
)
).some((shouldTransform) => shouldTransform);
Expand Down Expand Up @@ -182,7 +182,7 @@ export function getRequireResolver(extensions, detectCyclesAndConditional, curre
return { id: wrapId(childId, EXTERNAL_SUFFIX), allowProxy: false };
}
parentMeta.requires.push({ resolved, isConditional });
await analyzeRequiredModule(parentId, resolved, isConditional, rollupContext.load);
await analyzeRequiredModule(parentId, resolved, isConditional, rollupContext.load.bind(this);
return { id: childId, allowProxy: true };
})
);
Expand Down