ref(build): Convert `postbuild.sh` to node script #4828

Lms24 · 2022-03-31T09:32:49Z

This PR replaces the bash script postbuild.sh with the node script postbuild.ts which can be run with ts-node. This is done to conform with our decision to change all bash scripts to node s.t. they can be run on any platform (incl. Windows). Furthermore, it gives us the flexibility to add additional functionality in the future that is harder to code/read in bash.

Note that this PR is part of an ongoing effort to restructure our build directory structure and our build process. This script will be slightly adjusted in future PRs. The main goal of this PR is to convert it to node which will enable future adjustments.

Specifically, this PR:

clones the behaviour of postbuild.sh in postbuild.ts
adds the removal of volta and scripts properties in copied package.json
- TODO: This is something I want to discuss if we should do. Those properties are AFAIK not needed in the package.jsons we ship in our tarballs. The Angular library packager (ng-packagr) even warns users to not include scripts in shipped package.jsons due to a security risk.
adjusts calls to postbuild.sh to postbuild.ts
adjusts a comment to reference the node script

cc @timfish

ref: https://getsentry.atlassian.net/browse/WEB-766

add `postbuild.ts` node script which can be run with ts-node to replace the bash script `postbuild.sh`. This is done to conform with our decision to change all bash scripts to node s.t. they can be run on any platform (incl. Windows). Furthermore, it gives us the flexibility to make more adjustments that are harder to code/read in bash. * clone behaviour of `postbuild.sh` * add removal of `volta` and `scripts` properties in copied package.json * adjust calls to postbuild.sh to node script * adjust reference in comment to node script

timfish · 2022-03-31T10:56:09Z

I was going to suggest that path.join(process.cwd(), BUILD_DIR, 'package.json') is preferred over ${process.cwd()}/${BUILD_DIR}/package.json for proper cross platform support... however I just tested this PR on Windows and everything worked perfectly so that's obviously no longer the case 🤔

Lms24 · 2022-03-31T13:01:27Z

But it's probably nonetheless the cleaner way of specifying paths, I think. Thanks for bringing it up. Changed it to path.join()

lobsterkatie · 2022-03-31T18:15:14Z

I was going to suggest that path.join(process.cwd(), BUILD_DIR, 'package.json') is preferred over ${process.cwd()}/${BUILD_DIR}/package.json for proper cross platform support... however I just tested this PR on Windows and everything worked perfectly so that's obviously no longer the case 🤔

But it's probably nonetheless the cleaner way of specifying paths, I think. Thanks for bringing it up. Changed it to path.join()

Kind of a nit, but I generally prefer path.resolve() over path.join() because it gives you an absolute path rather than a relative one, which is I think is ever-so-slightly more bug-proof.

lobsterkatie · 2022-03-31T18:23:46Z

adds the removal of volta and scripts properties in copied package.json

TODO: This is something I want to discuss if we should do. Those properties are AFAIK not needed in the package.jsons we ship in our tarballs. The Angular library packager (ng-packagr) even warns users to not include scripts in shipped package.jsons due to a security risk.

Huh. I read both the npm post and the vulnerability doc to which it links, and I don't get how us including our package.json scripts opens us up to this. I'm not saying we can't delete them - clearly users don't need them - but I'm missing the link between that and this security hole.

timfish · 2022-03-31T18:32:58Z

Kind of a nit, but I generally prefer path.resolve() over path.join()

oh yeah, especially here.
Doesn't

path.join(process.cwd(), BUILD_DIR, 'package.json')

become?

path.resolve(BUILD_DIR, 'package.json')

The thing that scares me with path.resolve() is it's treatment of leading / in any element 😬

lobsterkatie

A few small suggestions (and mostly just wordsmithing at that), but otherwise looks good!

scripts/postbuild.ts

lobsterkatie · 2022-03-31T18:51:10Z

packages/browser/.npmignore

 # Info: the paths in this file are adjusted to match once this
 # file is copied to `./build`. This is done by a postbuild script
-# located in sentry-javascript/scripts/postbuild.sh
+# located in sentry-javascript/scripts/postbuild.ts


It won't let me do an official "suggestion", but:

# Info: the paths in this file are adjusted appropriately once this # file is copied to `./build`. This is done by the postbuild script # sentry-javascript/scripts/postbuild.ts.

Saves having to answer the question "match what?".

Thanks for the suggestion - this made me realize that the comment is slightly misleading. My initial comment makes it appear as if postbuild actively modifies the paths in .npmignore, which is not the case. I'll clarify that postbuild only copies this file and the paths are specified as if the file was already located in build.

* comment/spelling corrections * console output Co-authored-by: Katie Byers <[email protected]>

Lms24 · 2022-04-01T08:26:45Z

Thanks for the reviews!

Kind of a nit, but I generally prefer path.resolve() over path.join() because it gives you an absolute path rather than a relative one, which is I think is ever-so-slightly more bug-proof.

That's a fair point - I changed it to path.resolve(). I double checked locally and everything still works as expected.

I'm not saying we can't delete them - clearly users don't need them - but I'm missing the link between that and this security hole.

Agreed, I read through it, too (for the third time now) and admittedly I can't see how this would affect us. (Also on a side note, there isn't a lot of documentation available as to why e.g. the Angular team decided to remove scripts by default when building an Angular library besides the link to this post.) Anyway, the argument still stands that users do not need scripts (or volta for that matter) so I'm still in favour of removing them.

scripts/postbuild.ts

as suggested by @mrbaguvix

scripts/postbuild.ts

AbhiPrasad · 2022-04-04T09:57:34Z

scripts/postbuild.ts

+
+// modify entry points to point to correct paths (i.e. strip out the build directory)
+ENTRY_POINTS.filter(entryPoint => pkgJson[entryPoint]).forEach(entryPoint => {
+  pkgJson[entryPoint] = pkgJson[entryPoint].replace(`${BUILD_DIR}/`, '');


nit: filter + ForEach can just be a single reduce call, also means we emit a new object instead of mutating the previous reference.

also means we emit a new object instead of mutating the previous reference.

.filter() already creates a new array.

I also think that doing it in two stages like this makes it easier to reason about compared to using .reduce(). Of course, we could also always just do ENTRY_POINTS.forEach(entryPoint => if pkgJson[entryPoint] { <make value change> }}). Up to you, @Lms24.

.filter() already creates a new array.

yup, but important to note that one is more likely to cause bugs from mutating pkgJson than ENTRY_POINTS

Thanks for the feedback but I don't entirely understand the reasoning of changing this. I used the filter-then-foreach approach to make it clear what my intention is here (i.e. only change entry point paths on properties that in fact exist in the package.json file). Which IMHO (love to be proven wrong though) is a quite clean approach of operating on arrays. Also, I don't entirely see how reduce would be nicer here, could you explain?
Unless there's a good reason which I overlooked (which there very well might be), I'd leave it as is (also keeping in mind our moving fast approach...)

Something like so (please double check I wrote this in GH)

const newPkgJson = ENTRY_POINTS.reduce((acc, curr) => { acc[curr] = acc[curr].replace(`${BUILD_DIR}/`, ''); return acc; }, pkgJson)

Something like so (please double check I wrote this in GH)

const newPkgJson = ENTRY_POINTS.reduce((acc, curr) => { acc[curr] = acc[curr].replace(`${BUILD_DIR}/`, ''); return acc; }, pkgJson)

I find this significantly harder to understand as a reader, both in and of itself and as a way to communicate the intention here.

Ah interesting - I find it much easier to understand, it's just an array reducing to an object (a very typical FP operation) - I'm fine with leaving as is though.

Ahh got it, thanks. This would essentially not mutate the original pkgJson but create a new one with the changes instead - that's the intention, right? But wouldn't this solution still e.g. add a property to newPkgJson if curr didn't exist beforehand in pkgJson? If yes, then we'd need an if additionally, making it again less readable IMO.

I find this significantly harder to understand as a reader

Agreed, me too. Which is why I wanna go with the original version.

Go with your original, @Lms24. You have the power!

(This is what Daniel always used to say to me in such situations.)

it's just an array reducing to an object (a very typical FP operation)

@AbhiPrasad - perhaps that explains it. Because I haven't done a ton of FP, it doesn't hit my brain as, "Oh, that matches a pattern I know." Instead, my internal monologue is more like, "Okay, we're reducing, so we're combining stuff. How are we combining it? Oh, we're not, we're just fixing a string, but wait, is the string already there? Are we starting with something non-empty? acc kinda implies starting empty... Let's see - ah, okay, no, it's not empty. We're starting the the package.json data. Okay, but so wait, are we changing every entry? Where is curr coming from? The name doesn't tell me, so lemme look backwards... okay, yes, right - we're not iterating over package.json entries, we're iterating over ENTRY_POINTS. So curr is an entry point... ohhhh, okay, right. I get it. We're changing the value for all of the entry points."

vs

"Okay, we're taking the entry points, oh, but only the ones which exist in package.json, and then we're changing their value. Okay, got it."

Lms24 requested review from lforst and lobsterkatie March 31, 2022 09:32

use path.join() instead of template strings

f020d56

fix typo

34b038a

lobsterkatie reviewed Mar 31, 2022

View reviewed changes

Lms24 and others added 3 commits April 1, 2022 09:21

apply suggestions from code review

6aa4ff6

* comment/spelling corrections * console output Co-authored-by: Katie Byers <[email protected]>

clarify .npmignore comment, improve variable naming

d66c3d4

use path.resolve() instead of path.join()

312036d

Lms24 requested a review from lobsterkatie April 1, 2022 08:40

Lms24 mentioned this pull request Apr 1, 2022

ref(build): Introduce central build directory to packages with bundles #4838

Merged

0ex-d reviewed Apr 3, 2022

View reviewed changes

scripts/postbuild.ts Outdated Show resolved Hide resolved

add try/catch blocks to fs operations

38969e2

as suggested by @mrbaguvix

AbhiPrasad reviewed Apr 4, 2022

View reviewed changes

apply suggestions from code review

686644a

AbhiPrasad approved these changes Apr 4, 2022

View reviewed changes

Lms24 merged commit a064c7c into master Apr 5, 2022

Lms24 deleted the lms-convert-postbuild branch April 5, 2022 07:30

AbhiPrasad added this to the Pre 7.0.0 Work milestone Apr 6, 2022

Uh oh!

ref(build): Convert postbuild.sh to node script #4828

ref(build): Convert postbuild.sh to node script #4828

Uh oh!

Conversation

Lms24 commented Mar 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timfish commented Mar 31, 2022

Uh oh!

Lms24 commented Mar 31, 2022

Uh oh!

lobsterkatie commented Mar 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lobsterkatie commented Mar 31, 2022

Uh oh!

timfish commented Mar 31, 2022

Uh oh!

lobsterkatie left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lms24 Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lms24 commented Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AbhiPrasad Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lobsterkatie Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ref(build): Convert `postbuild.sh` to node script #4828

ref(build): Convert `postbuild.sh` to node script #4828

Lms24 commented Mar 31, 2022 •

edited

Loading

lobsterkatie commented Mar 31, 2022 •

edited

Loading

lobsterkatie left a comment •

edited

Loading

Lms24 Apr 1, 2022 •

edited

Loading

Lms24 commented Apr 1, 2022 •

edited

Loading

AbhiPrasad Apr 4, 2022 •

edited

Loading

lobsterkatie Apr 4, 2022 •

edited

Loading