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

build: update workflow to support ts #1119

Merged
merged 18 commits into from
May 5, 2023
Merged

build: update workflow to support ts #1119

merged 18 commits into from
May 5, 2023

Conversation

trim21
Copy link
Contributor

@trim21 trim21 commented Apr 9, 2023

This PR doesn't migrate source to TS except errors.ts as example ( or tester ), full migration and refacting can be done in seprated PRs.

It migrate source files to ESM and update tool config to support typescript.

close #834 #835

Code Style ( excluding already described in #1120 )

Add a set of eslint rules to force import style to make it work with build script.

import should be fully specified to import source files, developers of this project should write import {} from './errors.ts' for errors.ts, import {} from './helpers.js' for helpers.js, file extension must be included, this is essential for testing and building

function comment:

should always write jsdoc/tsdoc, tsc will only emit jsdoc/tsdoc in type definition. function/method comment // ... won't work.

Building

to not break existing require('minio/dist/main/...'), we still need to do a file to file transpiling.

There is no existing bundler can do this ( and tsc doesn't fit this case ), so a script build.mjs is added to use babel to transform codes.

src/ directory will be inclued in npm package but user should not be able to import then. editor (at lease vscode and WebStorm) will be able to jump to implementation (raw ts files in src/ dir) with tsconfig "declarationMap": true

Type definition are also imported from @types/minio, need some extra work to keep it up to date.

ESM

Also build esm mjs files to ./dist/esm/

Use babel to transpile type definition from .d.ts to .d.mts, so importing esm files will also be typed.

Add named export for all default export, and suggest stop make any new default export (keep existing backward compatible and remove them in next major version), use named exports only, like what we did in the package entrypoint. This is more clear to support both cjs and esm.

current building already transform esm default export to cjs default property, so no behavoir change here:

// current npm version
const r = require('minio/dist/main/CredentialProvider.js');

console.log(r.default);

now users can write code in a better way:

import { CredentialProvider } from 'minio/dist/esm/CredentialProvider.mjs';

const { CredentialProvider } = require('minio/dist/main/CredentialProvider.js');
console.log(new CredentialProvider(...));

// this still work, keep backward compatibility
const cp = require('minio/dist/main/CredentialProvider.js');
console.log(new cp.default(...));

Testing

still use mocha, no test cases are changed, but add @babel/register to load esm and ts files, so no need to compile before testing.

.env file is totally optional

mint

mint support minio/mint#356

About error.ts

Error.captureStackTrace should be used when you create a plain object {} as Error and want to add stack, not to be used when you extending built-in Error.

There is no need to call Error.captureStackTrace in the constructor of Error or sub-class of Erro, only need to set error.name.

Also it doesn't support es2022 standard error.cause (available in node16+, and doesn't broken lower version), there is no need to use it anymore.

@trim21 trim21 marked this pull request as draft April 9, 2023 16:24
@trim21 trim21 marked this pull request as ready for review April 9, 2023 19:51
@trim21 trim21 changed the title chore: update workflow to support ts build: update workflow to support ts Apr 9, 2023
Copy link
Contributor

@aldy505 aldy505 left a comment

Choose a reason for hiding this comment

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

LGTM

package.json Show resolved Hide resolved
@prakashsvmx
Copy link
Member

This is a great first step. Thank you for the contribution. 👍

Some comments:
The tests should be runnable locally and externally (mint link below)
hope we need to add .env or .env.local Please add and commit these files as well may be empty so others can follow it.

Code formatting can be taken up as a last step once we have all the other ts files migration.
We could add and generate the types once migration is complete, may be using https://github.com/Swatinem/rollup-plugin-dts

I still need to do the test/validate (changes if any) with https://github.com/minio/mint/tree/master/run/core/minio-js

@prakashsvmx
Copy link
Member

@trim21 Please resolve the conflicts.

@prakashsvmx
Copy link
Member

Another observation is : while running tests locally, the tests are not progressing after some point
e.g

....
 listIncompleteUploads removeIncompleteUpload
      ✔ initiateNewMultipartUpload(bucketName, objectName, metaData, cb)_bucketName:minio-js-test-d20bfea7-a775-460e-afa1-c1a04224e4cb, objectName:datafile-65-MB, metaData:[object Object]
      - listIncompleteUploads(bucketName, prefix, recursive)_bucketName:minio-js-test-d20bfea7-a775-460e-afa1-c1a04224e4cb, prefix:datafile-65-MB, recursive: true_
      - listIncompleteUploads(bucketName, prefix, recursive)_bucketName:minio-js-test-d20bfea7-a775-460e-afa1-c1a04224e4cb, recursive: true_
      - removeIncompleteUploads(bucketName, prefix)_bucketName:minio-js-test-d20bfea7-a775-460e-afa1-c1a04224e4cb, prefix:datafile-65-MB_
    fPutObject fGetObject
      ✔ fPutObject(bucketName, objectName, filePath, callback)_bucketName:minio-js-test-d20bfea7-a775-460e-afa1-c1a04224e4cb, objectName:datafile-65-MB, filePath:/tmp/datafile-65-MB_ (449ms)
      ✔ fPutObject(bucketName, objectName, filePath, metaData, callback)_bucketName:minio-js-test-d20bfea7-a775-460e-afa1-c1a04224e4cb, objectName:datafile-65-MB, filePath:/tmp/datafile-65-MB, metaData: [object Object]_ (323ms)

Steps to replicate:
add .env file with

SERVER_ENDPOINT="localhost:22000" 
ACCESS_KEY="minio" 
SECRET_KEY="minio123"
# Run Server in a terminal
CI=true  MINIO_ROOT_USER=minio MINIO_ROOT_PASSWORD=minio123 minio server /tmp/log{1...4}  --address ":22000" --console-address ":9012"

# Run build
rm -rf node_modules  dist && npm install && npm run build

# Run tests in another terminal
npm run test

@trim21
Copy link
Contributor Author

trim21 commented Apr 12, 2023

can we add Code formatting (#1120) first?would be easier for contributors to unify code formatting and pr diff could look better, easy for you to review following prs, too

lint-staged is setup so you won't need to format code manually

@trim21
Copy link
Contributor Author

trim21 commented Apr 12, 2023

The tests should be runnable locally and externally (mint link below)

I'll take a look at mint later

hope we need to add .env or .env.local Please add and commit these files as well may be empty so others can follow it.

I add a .env.example file, it would be better to not commit .env file into git so user can change it and have a different config

@trim21
Copy link
Contributor Author

trim21 commented Apr 12, 2023

Another observation is : while running tests locally, the tests are not progressing after some point

I find these tests become unexpected slow, I'll look into it

    fPutObject fGetObject
      √ fPutObject(bucketName, objectName, filePath, callback)_bucketName:minio-js-test-1ef393ec-7c3a-41d2-873e-323764721bd3, objectName:datafile-65-MB, filePath:C:\Users\Trim21\AppData\Local\Temp/datafile-65-MB_ (88069ms)
      √ fPutObject(bucketName, objectName, filePath, metaData, callback)_bucketName:minio-js-test-1ef393ec-7c3a-41d2-873e-323764721bd3, objectName:datafile-65-MB, filePath:C:\Users\Trim21\AppData\Local\Temp/datafile-65-MB, metaData: [object Object]_ (55277ms)
      √ fGetObject(bucketName, objectName, filePath, callback)_bucketName:minio-js-test-1ef393ec-7c3a-41d2-873e-323764721bd3, objectName:datafile-65-MB, filePath:C:\Users\Trim21\AppData\Local\Temp/datafile-65-MB.download_ (19084ms)
      √ removeObject(bucketName, objectName, filePath, callback)_bucketName:minio-js-test-1ef393ec-7c3a-41d2-873e-323764721bd3, objectName:datafile-65-MB_ (184ms)
      √ fPutObject(bucketName, objectName, filePath, metaData)_bucketName:minio-js-test-1ef393ec-7c3a-41d2-873e-323764721bd3, objectName:datafile-65-MB, filePath:C:\Users\Trim21\AppData\Local\Temp/datafile-65-MB_ (62562ms)
      √ fGetObject(bucketName, objectName, filePath)_bucketName:minio-js-test-1ef393ec-7c3a-41d2-873e-323764721bd3, objectName:datafile-65-MB, filePath:C:\Users\Trim21\AppData\Local\Temp/datafile-65-MB.download_ (16623ms)
      √ removeObject(bucketName, objectName, filePath, callback)_bucketName:minio-js-test-1ef393ec-7c3a-41d2-873e-323764721bd3, objectName:datafile-65-MB_ (191ms)

update:

I try master branch and tests freezed, too. It may not be the problme of this PR?

      √ putObject(bucketName, objectName, stream, metadata, cb)_bucketName:minio-js-test-de1a11fa-42fb-418e-a597-e988d74ba9bb, objectName:datafile-65-MB_ (104377ms)

you can see the log of ci trigged by PR #1124

2023-04-11T21:20:13.7722694Z     bucket notifications
2023-04-11T21:20:13.7727732Z       #listenBucketNotification
2023-04-11T21:20:13.7762002Z         ✓ listenBucketNotification(bucketName, prefix, suffix, events)_bucketName:minio-js-test-3847344e-6d30-4f33-ac74-7897d5cd3c0b, prefix:photos/, suffix:.jpg, events:bad_
2023-04-11T21:20:33.7929167Z         ✓ listenBucketNotification(bucketName, prefix, suffix, events)_bucketName:minio-js-test-3847344e-6d30-4f33-ac74-7897d5cd3c0b, events: s3:ObjectCreated:*_ (20016ms)
2023-04-11T21:20:44.8110284Z         ✓ listenBucketNotification(bucketName, prefix, suffix, events)_bucketName:minio-js-test-3847344e-6d30-4f33-ac74-7897d5cd3c0b, events:s3:ObjectRemoved:* (11018ms)

@prakashsvmx
Copy link
Member

prakashsvmx commented Apr 12, 2023

@trim21 the master the tests are running fine.

.....
    Force Deletion of prefix
      Test force removal of a prefix
        ✓ putObject(bucketName, objectName, stream)_bucketName:minio-js-fd-nv-e940a55c-9db6-43cf-9744-03d927484735, objectName:my-prefix/datafile-100-kB, stream:100Kib_
        ✓ removeObject(bucketName, objectList, removeOpts)_bucketName:minio-js-fd-nv-e940a55c-9db6-43cf-9744-03d927484735_Remove 1 object
        ✓ listObjects(bucketName, prefix, recursive)_bucketName:minio-js-fd-nv-e940a55c-9db6-43cf-9744-03d927484735, prefix: 'my-prefix', recursive:true_


  409 passing (34s)
  3 pending

@trim21
Copy link
Contributor Author

trim21 commented Apr 12, 2023

should have been fixed I think


  407 passing (44s)
  3 pending

@trim21 trim21 marked this pull request as draft April 12, 2023 14:22
@aldy505
Copy link
Contributor

aldy505 commented Apr 12, 2023

I wanted to suggest the usage of minio:cicd-edge docker image as a service on the CI, rather than running the minio server as a background process.

But the cicd-edge tag is far outdated and there's not much maintainers that wants to use it.

@trim21
Copy link
Contributor Author

trim21 commented Apr 12, 2023

I wanted to suggest the usage of minio:cicd-edge docker image as a service on the CI, rather than running the minio server as a background process.

But the cicd-edge tag is far outdated and there's not much maintainers that wants to use it.

I think this is not very related to this PR and can be a seprated issue ?

@aldy505
Copy link
Contributor

aldy505 commented Apr 12, 2023 via email

@trim21
Copy link
Contributor Author

trim21 commented Apr 12, 2023

Yeah, but it's something that I'd suggest on the Typescript rewrite journey. I'll turn my issue on Typescript into a checklist later on.

It can be parallel to the rewrite...

@trim21
Copy link
Contributor Author

trim21 commented Apr 12, 2023

I almost finish migrating, just wait style pr and this pr to be merged, then I can start to submit them

@trim21
Copy link
Contributor Author

trim21 commented May 2, 2023

This PR is ready for review now

@trim21
Copy link
Contributor Author

trim21 commented May 2, 2023

I'm not sure how the release bot will build this project, So I add npm run build to prepublishOnly script to make sure package will build

@trim21
Copy link
Contributor Author

trim21 commented May 2, 2023

mint support pull request created minio/mint#356

@prakashsvmx
Copy link
Member

mint support pull request created minio/mint#356

Thank you @trim21 . i was working in parallel on this. Sorry for not notifying you earlier.

@prakashsvmx prakashsvmx closed this May 2, 2023
@prakashsvmx
Copy link
Member

Accidentally closed this.

@prakashsvmx prakashsvmx reopened this May 2, 2023
@trim21
Copy link
Contributor Author

trim21 commented May 2, 2023

Accidentally closed this.

That's scaring, I thought you are working on this and decide to discard this PR. 😰

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.

add source maps to npm package
4 participants