Node: Remove compiled JavaScript from repository and compile TypeScript code on NPM prepare script on demand when installed via git

[ibc]

• Remove node/lib/ (this is, compiled JavaScript code) from the repository.
• Add prepare NPM script.
• When publishing NPM package it only contains node/lib/ (as before this PR).
• When people install mediasoup from GitHub via git, devDependencies are always installed (read NPM prepare docs) and TypeScript code is compiled to JS in prepare script.

BONUS TRACKS:

• Create package-lock.json so we ensure that mediasoup deps' versions are under our control.

[ibc]

Node CI on Windows fails because rmdir /s /q "node/lib" tries to delete a folder (lib) that doesn't exist:

https://github.com/versatica/mediasoup/actions/runs/3444484978/jobs/5747136816

Unfortunately rmdir doesn't provide with any flag to not fail if the folder doesn't exist. Any suggestion?

[jmillan]

I'll apply the same to flatbuffers branch in order to avoid the many JS autogenerated files in the repo.

[nazar-pc]

Unfortunately rmdir doesn't provide with any flag to not fail if the folder doesn't exist. Any suggestion?

Just ignore the error entirely, you don't have to fail.

Hm... you were so strongly against this before.
What is the motivation this time?

The way it is implemented seems pretty bad IMO because it forces everyone to have typescript in their production node_modules, which will lead to HUGE number of extra dependencies.

Also if you want to compile it at installation, you'll have to remove node/lib from files in package.json.

I'd vote against this approach, it needs to compile before publishing, you can even make CI do the publishing when you push a tag.

[ibc]

Hm... you were so strongly against this before.
What is the motivation this time?

Definitely the fact that having JS code in the repo is hell when it comes to review PRs.

The way it is implemented seems pretty bad IMO because it forces everyone to have typescript in their production node_modules, which will lead to HUGE number of extra dependencies.

And what is the problem? typescript package is 68.8 MB (unpacked size), 3.2 MB (minified), 828.3 kB (minified + gziped), and it has 0 dependencies:

• https://www.npmjs.com/package/typescript
• https://bundlephobia.com/package/[email protected]

We are already fetching larger dependencies (usrsctp subproject) in installation time.

Also if you want to compile it at installation, you'll have to remove node/lib from files in package.json.

No need for it. node/lib/ is already excluded by .gitignore in this PR.

Use a .npmignore file to keep stuff out of your package. If there's no .npmignore file, but there is a .gitignore file, then npm will ignore the stuff matched by the .gitignore file. If you want to include something that is excluded by your .gitignore file, you can create an empty .npmignore file to override it.

[jmillan]

I think it's cleaner to only have the source files in the repo. Having the user download and compile the TS (the same way we do with C++ worker) looks good to me.

We could, in the future, do this as part of the CI, if we find it worth it, but we are OK with no generating worker binaries too, so I'm not sure this would be required.

[nazar-pc]

And what is the problem? typescript package is 68.8 MB (unpacked size), 3.2 MB (minified), 828.3 kB (minified + gziped), and it has 0 dependencies:

I'm genuinely surprised by 0 dependencies, but 68.8M bigger node_modules is certainly something that would concern be quite a bit!

We are already fetching larger dependencies (usrsctp subproject) in installation time.

And clean it up after compilation, such that a single final executable is left in node_modules.

No need for it. node/lib/ is already excluded by .gitignore in this PR.

I don't see how those are connected, you're publishing from local machine anyway. package.json should contain an explicit list of files to be packaged before uploading to NPM. If you don't want node/lib to be on NPM you need to remove it from there, I see no reason why you'd still want something that isn't published there.

---

IMHO this needs to happen instead:

• node/src should be removed from NPM package
• compilation should be running with prepublish
• only node/lib is published to NPM (this is the build artifact people care about)

The only case when this is annoying is when people use github:repo/mediasoup as dependency, but then I guess they can use TypeScript directly.

What this PR does is functional, but it is an anti-pattern to include build dependencies in production dependencies.

[ibc]

I'm genuinely surprised by 0 dependencies, but 68.8M bigger node_modules is certainly something that would concern be quite a bit!

Well, pros and cons. We can delete node_modules/typescript/ as last step in postinstall, although I don't think it's very elegant.

No need for it. node/lib/ is already excluded by .gitignore in this PR.

I don't see how those are connected, you're publishing from local machine anyway. package.json should contain an explicit list of files to be packaged before uploading to NPM. If you don't want node/lib to be on NPM you need to remove it from there, I see no reason why you'd still want something that isn't published there.

Well, I insist we already rely on .gitignore for this. When you run npm install mediasoup it does not include any of the paths present in .gitignore:

## Meson.
/worker/subprojects/*
!/worker/subprojects/*.wrap

## Node.
/node_modules/

## Rust.
/Cargo.lock
/rust/examples-frontend/*/node_modules
/rust/examples-frontend/*/package-lock.json

## Worker.
/worker/out/
/worker/scripts/node_modules/
# Vistual Studio generated Stuff.
/worker/**/Debug/
/worker/**/Release/
/worker/.vs/
# clang-fuzzer stuff is too big.
/worker/deps/clang-fuzzer
# Ignore all fuzzer generated test inputs.
/worker/fuzzer/new-corpus/*
!/worker/fuzzer/new-corpus/.placeholder

## Others.
/coverage/
/NO_GIT/
*.swp
*.swo
.DS_Store
# Vistual Studio Code stuff.
/.vscode/
# JetBrains IDE stuff.
/.idea/

and we don't need to explicitly say in package.json which files/folder we want to include in the NPM package, right? So this is working for long and we rely on it since day 0. We don't need to add a files entry in package.json and we don't need to create a .npmignore file because the existing .gitignore works perfectly for us.

• only node/lib is published to NPM (this is the build artifact people care about)

The only case when this is annoying is when people use github:repo/mediasoup as dependency, but then I guess they can use TypeScript directly.

People (sometimes us) need to test a mediasoup branch within their application.. Unfortunately this is a very common use case and we cannot break it.

[nazar-pc]

Well, pros and cons. We can delete node_modules/typescript/ as last step in postinstall, although I don't think it's very elegant.

I understand that it is not, there are two reasons:

• NPM takes .gitignore into account

• mediasoup/package.json

  Lines 22 to 40 in 570cafd

  	"files": [
  	"node/lib",
  	"worker/deps/libwebrtc",
  	"worker/fuzzer/include",
  	"worker/fuzzer/src",
  	"worker/include",
  	"worker/scripts/*.py",
  	"worker/scripts/*.sh",
  	"worker/scripts/*.js",
  	"worker/scripts/*.json",
  	"worker/src",
  	"worker/subprojects/*.wrap",
  	"worker/test/include",
  	"worker/test/src",
  	"worker/Makefile",
  	"worker/meson.build",
  	"worker/meson_options.txt",
  	"npm-scripts.js"
  	],

  includes explicit list of files that should be packaged, even if you don't have .gitignore, only those files will be included

I have seen way too many packages that package random unnecessary files to NPM. .gitignore doesn't help with random temporary files you may have from debugging, etc. I frequently have log files, Heaptrack recordings and lots of other stuff in working directory from debugging. Not to mention we have Rust sources that do not need to be included in NPM package even though it is not in .gitignore for obvious reasons.

My point being it makes no sense to include path in package.json just to ignore it in .gitignore. Those are different tools for slightly different purposes, even though they are sometimes complementary.

People (sometimes us) need to test a mediasoup branch within their application.. Unfortunately this is a very common use case and we cannot break it.

Fair, yet I still believe that must be the default workflow. If you want so much to compile things during installation instead of downloading precompiled stuff, what about moving TypeScript into separate location similarly to worker/scripts/node_modules that we can use to download TypeScript, compile source and clean up TypeScript compiler afterwards.

[jmillan]

It would be nice to be able to ignore node/lib to from github (.gitignore) so compiled .js files are not in github.

The problem is then that such directory cannot be added to the npm package via files entry by definition https://docs.npmjs.com/cli/v9/configuring-npm/package-json#files

[ibc]

@nazar-pc I've removed worker/test/src and worker/test/include in files in package.json (in fact I pushed it to v3 branch: 6df9036) and now it fails to install via npm install git+https://github.com/versatica/mediasoup.git:

meson.build:274:0: ERROR: Include dir test/include does not exist.

So why is it? Yes, meson.build includes those removed folders (and others), but those are just for mediasoup_worker_test and mediasoup_worker_fuzzer tasks. Why are those required during normal installation of mediasoup-worker task?

[ibc]

@nazar-pc I've removed worker/test/src and worker/test/include in files in package.json (in fact I pushed it to v3 branch: 6df9036)

I've added them back in v3 in commit 461e0af

[ibc]

So this PR allows mediasoup installation via GH instead of using a NPM package:

npm install git+https://github.com/versatica/mediasoup.git#compile-typescript-in-npm-postinstall

This works fine.

[ibc]

Guys this is done and I'm super proud of the result. Nice and inspiring talk yesterday with @jmillan.

[nazar-pc]

This seems like actually idiomatic Node.js approach that combines all the features we want without the drawbacks previous attempt had.

Very nice, thanks for finally doing it 🎉

[ibc]

Waiting for final CI before merging.

Only thing that I don't love is that worker built stuff and subprojects are cleaned on NPM postinstall script which is executed when installing or updating a NPM dependency locally. I added a MEDIASOUP_LOCAL_DEV to avoid it:

$ MEDIASOUP_LOCAL_DEV=true npm install foo

Another solution is:

$ npm install --ignore-scripts foo

[nazar-pc]

I have added conditional code for that in Rust version too:

mediasoup/worker/build.rs

Lines 130 to 143 in 47147af

	if env::var("KEEP_BUILD_ARTIFACTS") != Ok("1".to_string()) {
	// Clean
	if !Command::new("make")
	.arg("clean-all")
	.env("MEDIASOUP_OUT_DIR", &mediasoup_out_dir)
	.spawn()
	.expect("Failed to start")
	.wait()
	.expect("Wasn't running")
	.success()
	{
	panic!("Failed to clean libmediasoup-worker")
	}
	}

[ibc]

Also in mediasoup-client: versatica/mediasoup-client#223