-
Notifications
You must be signed in to change notification settings - Fork 72
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
feat(bundle-source): Zip original sources with --no-transforms #2294
Conversation
9f6119c
to
61a24ef
Compare
Looking for review buddies for work pursuant to XS native compartments, #400. This change touches |
80fb0d5
to
bf2af00
Compare
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.
I had a vision... a prophecy. it was... merge conflicts
@@ -251,11 +275,13 @@ export const makeBundleCache = (wr, cwd, readPowers, opts) => { | |||
* @param {string} rootPath | |||
* @param {string} [targetName] | |||
* @param {Logger} [log] | |||
* @param {object} [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.
What is this type?
const { | ||
dev = false, | ||
cacheSourceMaps = false, | ||
noTransforms = false, | ||
commonDependencies, | ||
} = 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.
please add types for this function's parameters
export const parserForLanguage = { | ||
mjs: parserMjs, | ||
cjs: parserCjs, | ||
json: parserJson, | ||
text: parserText, | ||
bytes: parserBytes, |
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.
in case this informs anything you're doing here: I have a branch that has exposed user-defined languages and parsers, which seems like the least-awful way to get native modules running.
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.
I’m not going to land this before Tuesday next week. I do not mind rebuilding this on top of your more rigorous treatment. I’m not jazzed about exposing parserForLanguage
to the public API but it was sufficient and minimal for the bundleSource needs.
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.
I'm not going to land my code before Tuesday next week, so it's kind of moot. I originally had a parserForLanguage
option, but it turned out that I needed a new language, so it became an API that looks like:
parsers: [
{
languages: ['native'],
parser: someParserImpl,
extensions: ['node']
}
]
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.
I think I can make this work on top of your change without too much trouble. My intent is to make these -parsers.js
modules continue to export parserForLanguage
for internal use, but also languageForExtension
and parsers
like above that merges the two for public use. I can use the stub module to hide the implementation detail from the public.
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.
For those tuning in, we landed @boneskull’s work with some modifications to compose well with this line of work, and I removed the overlapping commits from this PR in the rebase. I then reconstructed the middle commits of this PR as #2306, as that was common ground for our separate concerns, and then removed the overlapping commits from this PR. All that remains now is the final integration in bundle-source, with very little modification from the changes that have been already approved here. The only big difference is that I’ve additionally decoupled the function that constructs a preliminary compartment map through exploration of node_modules
on a file system.
bf2af00
to
40ccdae
Compare
* @param {ArchiveOptions} [options] | ||
* @returns {Promise<Application>} | ||
*/ | ||
export const loadLocation = async (readPowers, moduleLocation, 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.
I'm used to default args:
export const loadLocation = async (readPowers, moduleLocation, options) => { | |
export const loadLocation = async (readPowers, moduleLocation, options = {}) => { |
const moduleText = new TextDecoder().decode(moduleBytes); | ||
const originalModuleText = await fs.promises.readFile(entryPath, 'utf-8'); | ||
// And, just to be sure, the text in the bundle matches the original text. | ||
t.is(moduleText, originalModuleText); |
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.
👏
Refs: #400 and #2294 ## Description In this change, we carve and export `-lite.js` and `-parsers.js` out of `import.js`, `archive.js`, and `import-archive.js`, as well revealing `mapNodeModules` through `@endo/compartment-mapper/node-modules.js` revealing hidden flexibility already present in the Compartment Mapper. This allows the Compartment Mapper to mix and match “language behaviors” with different workflows (e.g., using `import-parsers.js` with `archive-lite.js` instead of `archive-parsers.js` generates archives with original sources instead of precompiled sources.) We also decouple the process of discovering the physical compartments such that `node-modules.js` can be substituted for alternate compartment exploration algorithms, e.g., combining a custom lockfile and package cache from a specific package management solution. ### Security Considerations Does not cross security considerations. ### Scaling Considerations Exporting more narrowly scoped modules allows consumers to be avoid entraining Babel in a greater variety of scenarios. ### Documentation Considerations Evolution of the reference documentation should suffice, though another pass at README.md to dig into these more surgical and composable APIs may be worthwhile. ### Testing Considerations The existing scenarios are extensive and fully cover the behaviors that we’ve factored out. I expect #2294 tests to cover the new scenarios. ### Compatibility Considerations This change takes great care to preserve the existing behavior of the composite `import.js`, `archive.js`, and `import-archive.js` interfaces, only revealing existing behaviors that were previously internal, allowing more expressible scenarios. ### Upgrade Considerations This change will not affect upgrade and paves a migration path forward for preserving backward-compatibility for `bundleSource` and `importBundle` as we draw closer to supporting XS native compartments.
7b4bc52
to
e807ddf
Compare
e807ddf
to
9d28fb8
Compare
Fills in the usage and NEWS for #2294.
Closes: #2295
Refs: #400, #2252
Description
This change adds a mode to the
bundle-source
command with the initial flag--no-transforms
that generates “endo zip base64” style bundles without applying the module-to-program transform and SES shim censorship evasion transforms, such that the original files on disk appear in the zip file. This is a preparatory step, necessary for building test artifacts, in advance of full support for this bundle style.Security Considerations
bundle-source
is part of the Endo and Agoric toolkit and it, or its surrogate, participate in the toolchain for generating content that can be confined by Hardened JavaScript, but is not trusted by Hardened JavaScript at runtime. It does however currently run with all the authority of the developer in their development environment and its integrity must be carefully guarded.Scaling Considerations
No improvements expected at this time, but in pursuit of #400, it may be possible to move the heavy and performance sensitive JavaScript transform components from
bundle-source
toimport-bundle
and only suffer the performance cost of these transforms on Node.js, where those costs are more readily born by some runtimes. Precompiled bundles may continue to be the preferred medium for deployment to the web, for example.Documentation Considerations
We will need to advertise the
--no-transforms
flag eventually, since there will be a period where it is advisable if not necessary to generate contracts and caplets targeting the XS runtime.Testing Considerations
I have included a test that verifies the API behavior and manually run the following to verify behavior for the CLI:
Compatibility Considerations
This flag is opt-in and breaks no prior behaviors. This introduces a new entry to the build cache meta-data and may cause some bundles to be regenerated one extra time after upgrading.
Upgrade Considerations
This should not impact upgrade, though it participates in the greater #400 story which will require xsnap upgrades to come to bear.