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

Migrating to Typescript #1698

Merged
merged 7 commits into from
May 1, 2022
Merged

Migrating to Typescript #1698

merged 7 commits into from
May 1, 2022

Conversation

seiyab
Copy link
Contributor

@seiyab seiyab commented Jan 26, 2022

What this PR changes?

  • Migrates Flow -> TypeScript
  • Migrates standard -> ts-standard (Official variant for TypeScript)
  • Adds lint and type check for ui into build setting
  • Resolves (or sometimes ignores) type errors.

What I'm planning after this PR

  • Opening issue about any or ts-ignore so that digdag-ui will become more reliable
  • Opening issue about eslint-disable so that digdag-ui will become more relieable
  • Submitting PRs that resolve some parts of issues above

For reviewers, and for contributors who will come here from commit history

  • I didn't resolve many typing / lint problems so that I can work this PR feasibly
    • So any, ts-ignore and eslint-disable are not desired ones. Resolving them should be welcome

// because of the global type, this results in an eslint error (when it's not)
type ConsoleConfig = {
url: string;
interface AuthItem {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ts-standard did type -> interface

type ConsoleConfig = {
url: string;
interface AuthItem {
key: string
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ts-standard removed trailing , ; in type definitions

@seiyab seiyab changed the title [WIP] Migrating to Typescript Migrating to Typescript Feb 2, 2022
@seiyab
Copy link
Contributor Author

seiyab commented May 1, 2022

@szyn
Could you review this?
It is likely to conflict every digdag-ui change.

Copy link
Member

@szyn szyn left a comment

Choose a reason for hiding this comment

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

Sorry for being late to review this PR, it looks good to me.

@szyn szyn added this to the v0.10.5 milestone May 1, 2022
@szyn szyn merged commit 8b21fdc into treasure-data:master May 1, 2022
@seiyab seiyab deleted the typescript branch May 6, 2022 12:25
@seiyab
Copy link
Contributor Author

seiyab commented Jul 28, 2022

I found that current master fails on type check (./gradlew digdag-ui:checkUi)

console.tsx(2041,34): error TS2769: No overload matches this call.
  Overload 1 of 2, '(data: Data, options: InflateFunctionOptions & { to: "string"; }): string', gave the following error.
    Argument of type 'Data | null' is not assignable to parameter of type 'Data'.
      Type 'null' is not assignable to type 'Data'. 
  Overload 2 of 2, '(data: Data, options?: InflateFunctionOptions | undefined): Uint8Array', gave the following error.
    Argument of type 'Data | null' is not assignable to parameter of type 'Data'.

> Task :digdag-ui:checkUi FAILED

@szyn @yoyama
Should I add ./gradlew digdag-ui:checkUi or npm run type into GitHub actions?
I'm confused by another error in ./gradlew check.
I've supposed that ./gradlew check should run on CI.

> Task :digdag-standards:test                                                                                                                                                                               
                                                                                                                                                                                                            
io.digdag.standards.operator.td.TdBaseTdJobOperatorTest > initializationError FAILED                                                                                                                        
    org.objenesis.ObjenesisException at SunReflectionFactoryHelper.java:54                                                                                                                                  
        Caused by: java.lang.reflect.InvocationTargetException at NativeMethodAccessorImpl.java:-2
            Caused by: java.lang.IllegalAccessError at ClassLoader.java:-2
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by org.eclipse.jetty.util.BufferUtil (file:~/.gradle/caches/modules-2/files-2.1/org.eclipse.jetty/jetty-util/9.3.11.v20160721/1812ffd5a04698051180d582c146ca80
7760c808/jetty-util-9.3.11.v20160721.jar) to field java.nio.MappedByteBuffer.fd
WARNING: Please consider reporting this to the maintainers of org.eclipse.jetty.util.BufferUtil
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release

304 tests completed, 1 failed                       

> Task :digdag-standards:test FAILED

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.

None yet

2 participants