Skip to content
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

Rewrite extension scripts in TypeScript #598

Open
unflxw opened this issue Feb 4, 2022 · 7 comments
Open

Rewrite extension scripts in TypeScript #598

unflxw opened this issue Feb 4, 2022 · 7 comments

Comments

@unflxw
Copy link
Contributor

unflxw commented Feb 4, 2022

The extension install script is written in JavaScript, while the rest of our codebase is in TypeScript. If we could move this to TypeScript as well, it would streamline using bits of the codebase, such as the Transmitter, from the extension script.

We may want to reconsider whether the separation between the scripts and the regular source code is necessary. If we keep this separation, this may mean our build process calls the TypeScript compiler twice. There may be some TSConfig toggle that allows us to work around this.

@tombruijn
Copy link
Member

If we are able to configure TypeScript to compile everything with one config, the one thing we have to watch out for is that users can't actually import or run the extension install script files. They should never be accessible outside of the package.

@unflxw
Copy link
Contributor Author

unflxw commented Feb 4, 2022

the one thing we have to watch out for is that users can't actually import or run the extension install script files.

While looking into an issue regarding making the extension use the transmitter (which relates to it, as it's in TS), I realised that the above is already the case in our current releases:

const reportScript = require("@appsignal/nodejs/scripts/extension/report")
console.log("reportPath", reportScript.reportPath())

@tombruijn
Copy link
Member

tombruijn commented Feb 7, 2022

Oh that's maybe not the best setup. We can move the function to the src dir and import in the install script instead? Turn it around. Wait, I can't find it. We duplicate the reportPath function at the moment.

@tombruijn
Copy link
Member

As just discussed in the call, this would not work for the local setup. Not from scratch. If you clone the repo, run mono bootstrap, then it will run npm run install on the @appsignal/nodejs package, which won't have the TypeScript compiled files yet.

@unflxw
Copy link
Contributor Author

unflxw commented Feb 7, 2022

That's true, but we could have npm install run npm run build when it's a local build. The branch where the extension uses the transmitter does this to work around it.

I think the above issue, with the scripts being importable, is essentially inevitable. I've checked node-sass, which also has scripts, and the same thing happens. require() allows you to access any file in the modules, and I don't know if there's a way around that. That said, the changes to the extension in #600 should at least make it so that requiring it doesn't automatically run the install process.

@tombruijn
Copy link
Member

Let's postpone work on it for a bit until we've completed the work on the extension install script and are a bit more sure what we want here.

@tombruijn
Copy link
Member

Noemi and I discussed fixing the mono bootstrap flow that would make this whole thing less confusing.

mono bootstrap currently calls npm run install which installs the extension. If we use TypeScript in the extension install script you need to build the TypeScript to JavaScript first.

What if instead we don't run npm run install on mono bootstrap locally? But run npm run install after npm run build using mono (using a post build hook) or package.json (postbuild: "npm run install"). That way it's not automatically installed on mono bootstrap and we don't have this build order issue locally.

This would only be for local builds, because the distributed package does need to have this flow, but it ships with the TypeScript compiled to JavaScript.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants