search

finos / FDC3

An open standard for the financial desktop.
https://fdc3.finos.org
Other
187 stars 109 forks source link

Remove TSDX from build system #1175

Closed julianna-ciq closed 2 months ago

julianna-ciq commented 3 months ago

resolves #1061

TSDX is a build system to uses rollup, prettier, eslint, and jest. It's helpful to use TSDX, because it will manage your build, lint, and testing procedures for you, instead of you having to manage all of those dependencies.

Unfortunately, TSDX hasn't been keeping those dependencies up-to-date, including the dependencies with vulnerabilities. If you have already cloned this repo, go to the main branch and run npm audit. You'll see 17 moderate vulnerabilities. Those vulnerabilities are all downstream of TSDX, and many of them are for features not even used in this repo. Furthermore, since TSDX relies on so many stale dependencies, it's reasonable to expect that problems will only continue to grow .

To overcome the risk presented by TSDX's stale dependencies, I've replaced TSDX with the rollup, prettier/eslint, and jest dependencies that it was obfuscating. Note that I copied over most of TSDX's configs for these tools, to maintain continuity.

The changes in this PR include:

linux-foundation-easycla[bot] commented 3 months ago

CLA Signed

The committers listed above are authorized under a signed CLA.

netlify[bot] commented 3 months ago

Deploy Preview for fdc3 ready!

Name Link
Latest commit 27056d44732ff19680b3a3bd06a7cba9e3f38864
Latest deploy log https://app.netlify.com/sites/fdc3/deploys/6630e48c316c670008444262
Deploy Preview https://deploy-preview-1175--fdc3.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

bingenito commented 2 months ago

Tested all changed commands on windows (command prompt) and RHEL9 and look good. Also very happy to see npm install complete with 0 vulnerabilities.

kriswest commented 2 months ago

We only had a couple of minor types conflicts in finsemble when we dropped the new module in - one relates to a change that should only go out in 2.2.0 that's already merged and the second maybe a type issue caused by that first one. Hence, I'll figure out what to do to back out the change temporarily so we can get this released!

kriswest commented 2 months ago

The change that needs backing out is:

bingenito commented 2 months ago

Did it always build upon npm install? I now get eslint failures that I wasn't seeing on last test -

> @finos/fdc3@2.1.0-beta.8 lint
> eslint src/ test/ --ext .ts --fix

C:\Users\brian\Source\FDC3\src\context\ContextType.ts
  46:85  error  Don't use `{}` as a type. `{}` actually means "any non-nullish value".
- If you want a type meaning "any object", you probably want `object` instead.
- If you want a type meaning "any value", you probably want `unknown` instead.
- If you want a type meaning "empty object", you probably want `Record<string, never>` instead.
- If you really want a type meaning "any non-nullish value", you probably want `NonNullable<unknown>` instead  @typescript-eslint/ban-types

C:\Users\brian\Source\FDC3\src\intents\Intents.ts
  32:49  error  Don't use `{}` as a type. `{}` actually means "any non-nullish value".
- If you want a type meaning "any object", you probably want `object` instead.
- If you want a type meaning "any value", you probably want `unknown` instead.
- If you want a type meaning "empty object", you probably want `Record<string, never>` instead.
- If you really want a type meaning "any non-nullish value", you probably want `NonNullable<unknown>` instead  @typescript-eslint/ban-types

✖ 2 problems (2 errors, 0 warnings)
kriswest commented 2 months ago

It did indeed always build on install, not sure why.

These two new eslint errors are from:

We'll should adjust as it suggests to clear them out...

kriswest commented 2 months ago

@bingenito @andreifloricel it took me a little while to understand what was going on here (https://github.com/InteropIO/FDC3/blob/c2cb9d51e8644ac64e3640b1eb0cb95bb9cc8d5b/src/context/ContextType.ts#L46-L48).

It would be nice if, when using the type, if Intellisense or similar was proposing the standard types for you - although you can create your own so any string is allowed:

export type ContextType = StandardContextType | ExperimentalContextType | string;

However, the typescript compiler aggressively reduces that to just string, which is correct for type safety, but useless for autocomplete. @andreifloricel clearly knows this and used the standard workaround

export type ContextType = StandardContextType | ExperimentalContextType | (string & {});

However, that trips the modern linting rule @typescript-eslint/ban-types. I couldn't find another composition that achieves the same, so I've just disabled that linting rule for the two lines concerned, and all is well again.

kriswest commented 2 months ago

@bingenito latest version should no longer typegen & build on npm install. Switched to prepack from prepare.

Also @julianna-ciq fixed the missing dist/index.js file, which was being generated by a custom feature (custom rollup plugin) of TSDX:

https://www.unpkg.com/browse/@finos/fdc3@2.1.0-beta.6/dist/index.js <- TSDX https://www.unpkg.com/browse/@finos/fdc3@2.1.0-beta.9/dist/index.js <- new build