Skip to content

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

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

Merged
merged 8 commits into from
Apr 5, 2022
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
80ab1be
ref(build): convert `postbuild.sh` to node script
Lms24 Mar 31, 2022
f020d56
use `path.join()` instead of template strings
Lms24 Mar 31, 2022
34b038a
fix typo
Lms24 Mar 31, 2022
6aa4ff6
apply suggestions from code review
Lms24 Apr 1, 2022
d66c3d4
clarify `.npmignore` comment, improve variable naming
Lms24 Apr 1, 2022
312036d
use `path.resolve()` instead of `path.join()`
Lms24 Apr 1, 2022
38969e2
add try/catch blocks to fs operations
Lms24 Apr 4, 2022
686644a
apply suggestions from code review
Lms24 Apr 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions packages/browser/.npmignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# 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
# Info: the paths in this file are specified so that they align with the file
# structure in `./build` where this file is copied to. This is done by the
# postbuild script `sentry-javascript/scripts/postbuild.ts`.

*

Expand Down
2 changes: 1 addition & 1 deletion packages/browser/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
"webpack": "^4.30.0"
},
"scripts": {
"build": "run-p build:cjs build:esm build:bundle build:types && bash ../../scripts/postbuild.sh",
"build": "run-p build:cjs build:esm build:bundle build:types && ts-node ../../scripts/postbuild.ts",
"build:bundle": "rollup --config",
"build:cjs": "tsc -p tsconfig.cjs.json",
"build:dev": "run-p build:cjs build:esm build:types",
Expand Down
40 changes: 0 additions & 40 deletions scripts/postbuild.sh
View file
Open in desktop

This file was deleted.

61 changes: 61 additions & 0 deletions scripts/postbuild.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/*
This script prepares the central `build` directory for NPM package creation.
It first copies all non-code files into the `build` directory, including `package.json`, which
is edited to adjust entry point paths. These corrections are performed so that the paths align with
the directory structure inside `build`.
*/

import * as fs from 'fs';

import * as path from 'path';

const BUILD_DIR = 'build';
const ASSETS = ['README.md', 'LICENSE', 'package.json', '.npmignore'];
const ENTRY_POINTS = ['main', 'module', 'types'];

// check if build dir exists
try {
if (!fs.existsSync(path.resolve(BUILD_DIR))) {
console.error(`Directory ${BUILD_DIR} DOES NOT exist`);
console.error("This script should only be executed after you've run `yarn build`.");
process.exit(1);
}
} catch (error) {
console.error(`Error while looking up directory ${BUILD_DIR}`);
}

// copy non-code assets to build dir
ASSETS.forEach(asset => {
const assetPath = path.resolve(asset);
try {
if (!fs.existsSync(assetPath)) {
console.error(`Asset ${asset} does not exist.`);
process.exit(1);
}
fs.copyFileSync(assetPath, path.resolve(BUILD_DIR, asset));
} catch (error) {
console.error(`Error while copying ${asset} to ${BUILD_DIR}`);
}
});

// package.json modifications
const packageJsonPath = path.resolve(BUILD_DIR, 'package.json');
const pkgJson: { [key: string]: string } = require(packageJsonPath);

// 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}/`, '');
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@AbhiPrasad AbhiPrasad Apr 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

.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

Copy link
Member Author

Choose a reason for hiding this comment

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

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...)

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@lobsterkatie lobsterkatie Apr 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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."

});

// TODO decide if we want this:
delete pkgJson.scripts;
delete pkgJson.volta;

// write modified package.json to file (pretty-printed with 2 spaces)
try {
fs.writeFileSync(packageJsonPath, JSON.stringify(pkgJson, null, 2));
} catch (error) {
console.error(`Error while writing package.json to disk`);
}

console.log(`\nSuccessfully finished postbuild commands for ${pkgJson.name}`);