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

fix(bundle-source): assert that the entrypoint exists at all #1256

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

warner
Copy link
Contributor

@warner warner commented Aug 16, 2022

closes #1206

@warner warner requested a review from kriskowal August 16, 2022 21:01
@warner
Copy link
Contributor Author

warner commented Aug 16, 2022

@kriskowal this is "draft" because the patch is insufficient in at least the following ways:

  • the fs.existsSync() call should probably be coming from powers, if something suitable exists therein
  • existsSync would be a funny power to provided when read is the actually useful one
  • that call does not understand e.g. ../demo/bigint as meaning ../demo/bigint.js

but I'm hoping you might be able to point me in the right direction so I can make it better. What's the right sort of way to build the assertion? Would it be easier to build in bundle{ZipBase64,NestedEvaluateAndGetExports), rather than up front in bundleSource?

(incidentally, patching my agoric-sdk copy with this code was enough to help figure out where I had a typo in my bundleSource() invocation, which was my most immediate goal)

@@ -402,6 +402,7 @@ export default async function bundleSource(
options = {},
powers = undefined,
) {
assert(fs.existsSync(startFilename));
Copy link
Contributor

Choose a reason for hiding this comment

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

un-requested feedback: I still would prefer avoiding Sync APIs where possible. In this case stat and checking for an ENOENT error is the alternative

@kriskowal
Copy link
Member

I’m sure there’s a more correct way to ensure that the file exists within compartment-mapper. exists is one of those “considered harmful” syscalls because it isn’t proof against race conditions on a non-transactional filesystem. existsSync is “considered harmful” because it limits portability to the web (perhaps not a concern specifically for bundleSource which has a higher hill to climb before it is portable).

The mystery is how it’s possible for compartment-mapper to construct a bundle at all if it cannot find the entrypoint module. This is probably due to the .catch(() => undefined) in the loop that searches for a file to satisfy an import specifier in the archiver’s importHook, and probably some missing logic to construct an appropriate error if the loop falls through. But, we see that error pretty frequently in other cases, so it must be a problem specific to the entrypoint module, specific to makeArchive, that allows it to produce an empty zip instead of throwing.

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.

bundleSource emits a bundle when entrypoint does not exist
3 participants