-
Notifications
You must be signed in to change notification settings - Fork 74
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(ses): add SES version to lockdown shim #1854
feat(ses): add SES version to lockdown shim #1854
Conversation
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.
Feedback for your consideration. If you are unable to merge this, please reply when it’s ready to merge.
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.
Looks fine, but had a question or two
packages/ses/scripts/bundle.js
Outdated
const bytes = await fs.promises.readFile( | ||
fileURLToPath(`${root}/package.json`), | ||
); | ||
const text = textDecoder.decode(bytes); |
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.
Can we not do this:
const bytes = await fs.promises.readFile( | |
fileURLToPath(`${root}/package.json`), | |
); | |
const text = textDecoder.decode(bytes); | |
const text = await fs.promises.readFile( | |
fileURLToPath(`${root}/package.json`), | |
'utf8' | |
); |
and drop the TextDecoder
? Is this an Endo convention?
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.
nice the 2nd arg w utf8
cleans up the need for TextDecoder
^
spotted utf-8
is also used, but don't think it matters? (same result)
if not, then RE convention i favour our suggestion (the default)
edit: i guess the same change could be made as follow-up in packages/check-bundle/index.js
and possibly deeper in various other parts the codebase
edit: just realised could do this aswell 🤷♂️
const bytes = await fs.promises.readFile(
fileURLToPath(`${root}/package.json`),
);
const packageJson = JSON.parse(bytes);
Changes made, re-ready 2 merge @kriskowal (i'm not authorized to merge) |
273db98
to
02e8896
Compare
Tested - `npm run build` - `head dist/ses.cjs`
Co-authored-by: Christopher Hiller <boneskull@boneskull.com>
02e8896
to
a3c3cc8
Compare
closes: #1853
Description
Prepends the
ses
package.json > version as a comment to the bundled lockdown fileSecurity Considerations
Scaling Considerations
Documentation Considerations
Possibly worth mentioning the version number is now included in the SES lockdown shim
Testing Considerations
Possibly add a unit test to check the version number is present at the head of the file
Manually tested locally:
nvm use 16 # or 18 or 20
npm i --force # https://github.com/endojs/endo/assets/1881059/72a4079c-fe00-4de8-ac2d-fb13406edbad
cd packages/ses npm run build
head dist/ses.cjs # ensure correct version present as comment on line 1
npm run test
on Node
Upgrade Considerations