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

feat: tsup, pnpm and new build system #565

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

productdevbook
Copy link

@productdevbook productdevbook commented May 13, 2023

close #413
close #459

  • dynamic build -> export dist
  • pnpm support
  • esm, cjs support

@@ -138,7 +138,7 @@ export async function assertThatBodyParserHasBeenUsedForExpressLikeRequest(
let err = await new Promise((resolve) => {
let resolvedCalled = false;
if (request.readable) {
jsonParser(request, new ServerResponse(request), (e) => {
jsonParser(request, new ServerResponse(request), (e: any) => {
Copy link
Member

Choose a reason for hiding this comment

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

can we not use any type?

Copy link
Author

Choose a reason for hiding this comment

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

you can use, i did any as i don't know what types are

@@ -71,7 +71,10 @@ function defaultCreateAndSendCustomSms(_: NormalisedAppinfo) {
* if the error is thrown from API, the response object
* will be of type `{err: string}`
*/
// TODO: check if this is the correct way to check for error
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

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

can we not add @ts-ignore to so many files?

Copy link
Author

Choose a reason for hiding this comment

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

there were ts errors in a few places only during the build, I closed them temporarily with this. We can do this better in the next pr.

@rishabhpoddar
Copy link
Member

hey @productdevbook could you also tell if this a breaking change for the user? For example, will the import statements for the user change in any way for any recipe / function etc?

@productdevbook
Copy link
Author

It should not affect it in any way, because I took the same paths. I can even create a playground for testing purposes.

@productdevbook
Copy link
Author

image

@rishabhpoddar
Copy link
Member

rishabhpoddar commented May 14, 2023

Okay. Im going to try and remove all the newly added @ts-ignore annotations before merging this PR.

Also, i see the you have removed the build folder and instead using dist, but dist itself is not part of the git repo (which is fine). This would affect the npm publish process right? Also, if someone is using the github repo directly in the npm command (like npm i git+https://github.com:supertokens/supertokens-node#14.0), how would they go about generating the required dist folder (for the package to work)

"build-check": "cd lib && npx tsc -p tsconfig.json --noEmit && cd ../test/with-typescript && npx tsc -p tsconfig.json --noEmit",
"build": "cd lib && rm -rf build && npx tsc -p tsconfig.json && cd ../test/with-typescript && npx tsc -p tsconfig.json --noEmit && cd ../.. && npm run post-build",
"build-check": "npx tsc -p tsconfig.json --noEmit && cd ../test/with-typescript && npx tsc -p tsconfig.json --noEmit",
"build": "tsup",
Copy link
Member

Choose a reason for hiding this comment

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

the build command earlier used to also check the compilation of ../test/with-typescript - this was done as a way to make sure that the interface exposed to the user doesn't break unintentionally.

Copy link
Author

Choose a reason for hiding this comment

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

you don't need them, tsup sets them all in dist. There is no need for that test, since that user will no longer have ts errors.

@productdevbook
Copy link
Author

productdevbook commented May 14, 2023

Okay. Im going to try and remove all the newly added @ts-ignore annotations before merging this PR.

Also, i see the you have removed the build folder and instead using dist, but dist itself is not part of the git repo (which is fine). This would affect the npm publish process right? Also, if someone is using the github repo directly in the npm command (like npm i git+https://github.com:supertokens/supertokens-node#14.0), how would they go about generating the required dist folder (for the package to work)

yes, used dist. example npm publish first build working and after send. package.json in

  "files": [
        "dist",
        "LICENSE",
        "README.md"
    ],

only send data.

@productdevbook
Copy link
Author

npm i git+https://github.com:supertokens/supertokens-node#14.0

I do not know about this. normally it should not be kept on dist vs github in any way.

@rishabhpoddar
Copy link
Member

I do not know about this. normally it should not be kept on dist vs github in any way.

Right, we do have requirements that we need the sdk to work even with directly using the git repo. Therefore, we would need the dist folder to be a part of the github repo as well (which is why earlier we had the build folder as well).

If you can think of a better solution to this, it would be great.

@productdevbook
Copy link
Author

productdevbook commented May 15, 2023

I can't find a solution to this, people who will use it will pr and create and export their github dist using their own url. This should not be in the main library. The solution is actually that simple. We already do the same thing in many libraries.

  1. fork and build and push. and me github url copy and paste.

@rishabhpoddar
Copy link
Member

I'm afraid that won't work for us. Maybe we can add a post npm install hook to do the TS compilation for such users. But we do require some solution that universally works even when pulling the GH repo directly. This is a hard requirement for us i'm afraid.

@productdevbook
Copy link
Author

Since it will be an npm package and it is a suitable external build, people will use it according to the package versions anyway. In addition, users who want to use github url, which is a very special situation, should do as I said. It's a must. There is a postinstall structure in npm, but it is very inefficient in this structure.

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

Successfully merging this pull request may close these issues.

bug: esm support Use Node.js 14 tsconfig configuration
2 participants