Add Typescript deps and build #148

shekharkhedekar · 2022-05-01T16:19:04Z

Overview

Preparation for Typescript

updates "main" target
adds "build" and adds "watch" script
Adds dependencies for 3rd party types (will be used in a follow up PR)
Adds tsconfig

Progress towards closing #136

Checks

✅ npm test
✅ npm link into my app and successfully deployed

simonh1000 · 2022-05-02T05:37:08Z

Nice - you've already addressed a concern that I had about including the build directory.

I want to research how best to publish a typescript based app

clearly we need a compiled (and minified?) JS export for JS users
at present, when you use the code, you use the source js code. That would be the future way typescript users use the code I think.
in the short run I think that means the type script would be exposed for import while the js is exposed for require - perhaps we bump major versions now and switch to ES6 exports for both

I don't know enough about JS testing so what is the reason for changing the test suite?

shekharkhedekar · 2022-05-02T16:11:59Z

Nice - you've already addressed a concern that I had about including the build directory.

I want to research how best to publish a typescript based app

clearly we need a compiled (and minified?) JS export for JS users

at present, when you use the code, you use the source js code. That would be the future way typescript users use the code I think.

in the short run I think that means the type script would be exposed for import while the js is exposed for require - perhaps we bump major versions now and switch to ES6 exports for both

I don't know enough about JS testing so what is the reason for changing the test suite?

RE: JS/TS - the JS projects will import build/ftp-deploy.js - the TS projects will do the same but import from ftp-deploy.d.ts for typings.

RE: Test frameworks, Jest has some easy TS integrations, and a nice coverage option.

shekharkhedekar · 2022-05-06T04:55:01Z

@simonh1000 lmk if you have any other feedback - I've got a few more PRs to get this TS conversion over the line.

simonh1000 · 2022-05-06T06:12:28Z

I'll try to take a look at the weekend

simonh1000 · 2022-05-07T07:29:46Z

src/ftp-deploy.spec.js

                return statP(remoteDir + "/test-inside-root.txt");
            })
-            .catch((err) => done(err));
+            .catch((err) => Promise.reject(err));


we do not need this line at all then?

simonh1000 · 2022-05-07T07:36:13Z

I tried to run the tests and got

Error: Jest: Failed to parse the TypeScript config file /Users/simonhampton/code/projects/ftp-deploy/jest.config.ts
Error: Jest: 'ts-node' is required for the TypeScript configuration files. Make sure it is installed
Error: Cannot find module 'ts-node'

simonh1000 · 2022-05-07T07:37:49Z

test/server.js

 const port = 2121;
-const homeDir =
-    require("os").homedir() + "/code/projects/ftp-deploy/test/remote";
+const homeDir = path.join(__dirname, "../test/remote");


I made this change so that people on windows could run the tests - is there a reason for deleting it?

Yes, my code is in ~/src. What if we used require("os").tmpdir() instead?

Actually, if I'm not mistaken, making this consistent with this should work the same on windows/mac node?

Can we revert this change please. I explained why before

Yes, reverted.

simonh1000 · 2022-05-07T07:39:45Z

to be honest it feels strange to add all this typescript config stuff without there being any typescript code yet. If I merged and someone was to look at the repo then, they would wonder what on earth is happening. Can we add the TS config with at least some TS code?

shekharkhedekar · 2022-05-08T21:02:42Z

I tried to run the tests and got

Error: Jest: Failed to parse the TypeScript config file /Users/simonhampton/code/projects/ftp-deploy/jest.config.ts
Error: Jest: 'ts-node' is required for the TypeScript configuration files. Make sure it is installed
Error: Cannot find module 'ts-node'

I was able to run the tests after pulling, and rm -rf node_modules/ && rm package-lock.json && npm i

shekharkhedekar · 2022-05-08T21:03:31Z

to be honest it feels strange to add all this typescript config stuff without there being any typescript code yet. If I merged and someone was to look at the repo then, they would wonder what on earth is happening. Can we add the TS config with at least some TS code?

Agreed. What if we used this as a base branch to merge all the subsequent TS PRs into?

simonh1000 · 2022-05-09T06:01:08Z

I was able to run the tests after pulling, and rm -rf node_modules/ && rm package-lock.json && npm i

I tried again with the same issue. I think you have a global install (of ts-node?) that i do not have

shekharkhedekar · 2022-05-09T15:17:25Z

I was able to run the tests after pulling, and rm -rf node_modules/ && rm package-lock.json && npm i

I tried again with the same issue. I think you have a global install (of ts-node?) that i do not have

I was able to repro the error with this (after removing the global install of ts-node:

rm -rf node_modules && rm package-lock.json && npm i && npm test

and verify that it was fixed with this

npm i -D ts-node && npm test

shekharkhedekar · 2022-05-30T00:00:24Z

@simonh1000 let me know if you have any updates

simonh1000 · 2022-05-30T09:58:04Z

src/ftp-deploy.spec.js

            .then(() => {
                // Should reject if file does not exist
                return statP(remoteDir + "/test-inside-root.txt");
-            })


what is the intention here?

removed this, it was an unnecessary change

…p-deploy into typescript-build-deps

build/ftp-deploy.js

build/lib.js

shekharkhedekar · 2022-05-08T21:00:22Z

test/server.js

 const port = 2121;
-const homeDir =
-    require("os").homedir() + "/code/projects/ftp-deploy/test/remote";
+const homeDir = path.join(__dirname, "../test/remote");


Yes, my code is in ~/src. What if we used require("os").tmpdir() instead?

shekharkhedekar · 2022-05-08T21:06:28Z

test/server.js

 const port = 2121;
-const homeDir =
-    require("os").homedir() + "/code/projects/ftp-deploy/test/remote";
+const homeDir = path.join(__dirname, "../test/remote");


Actually, if I'm not mistaken, making this consistent with this should work the same on windows/mac node?

shekharkhedekar · 2022-05-08T21:15:42Z

src/ftp-deploy.spec.js

                return statP(remoteDir + "/test-inside-root.txt");
            })
-            .catch((err) => done(err));
+            .catch((err) => Promise.reject(err));


shekharkhedekar · 2022-08-09T15:44:42Z

test/server.js

 const port = 2121;
-const homeDir =
-    require("os").homedir() + "/code/projects/ftp-deploy/test/remote";
+const homeDir = path.join(__dirname, "../test/remote");


Yes, reverted.

shekharkhedekar · 2022-08-10T01:59:42Z

src/ftp-deploy.spec.js

            .then(() => {
                // Should reject if file does not exist
                return statP(remoteDir + "/test-inside-root.txt");
-            })


removed this, it was an unnecessary change

shekharkhedekar · 2022-08-10T02:01:51Z

@simonh1000 sorry for the delays on this - lmk if you have more feedback

simonh1000 · 2022-08-18T06:13:18Z

Thanks - I will take a look soon but things are a bit busy at the weekend, which is the only time I have available for open source. Thansk for your patience

DEPSTRCZ · 2023-05-11T11:14:24Z

Thanks - I will take a look soon but things are a bit busy at the weekend, which is the only time I have available for open source. Thansk for your patience

Any updates on this?

simonh1000 · 2023-05-13T06:09:11Z

playground/test_script.js

please don't remove this file

simonh1000 · 2023-05-13T06:09:49Z

src/lib.spec.js

 const path = require("path");
 var assert = require("assert");
-
-const expect = require("chai").expect;


please do not change test framework without further justification

simonh1000 · 2023-05-13T06:10:35Z

jest.config.ts

@@ -0,0 +1,9 @@
+import type { Config } from "@jest/types";


please dont change test framework in a PR about typescript

simonh1000

See comments below - let's stay focused on the TS part

shekharkhedekar · 2024-01-13T18:52:22Z

closing in favor of #170

simonh1000 self-requested a review May 1, 2022 16:40

shekharkhedekar force-pushed the typescript-build-deps branch 2 times, most recently from 7c89d07 to 985afa4 Compare May 2, 2022 16:10

simonh1000 reviewed May 7, 2022

View reviewed changes

shekharkhedekar and others added 8 commits May 8, 2022 14:10

Add TS deps and build

267f1b3

build folder, fix npm scripts and config

bff58ff

Update tsconfig.json

c00028d

Move to jest

9ab5072

fix imports

28d5f34

add prepublish script

19c7ff6

Add coverage, remove types for now

978f5c1

Add ts-node

c170e24

shekharkhedekar force-pushed the typescript-build-deps branch from bb5d9f1 to c170e24 Compare May 8, 2022 21:14

Remove unnecessary catch clause

8e7e30e

move ts deps to devDependencies

dc82980

simonh1000 reviewed May 30, 2022

View reviewed changes

simonh1000 added 2 commits July 5, 2022 08:31

update deps

d0a5264

bump

bffc90b

shekharkhedekar and others added 14 commits August 9, 2022 08:43

Revert homedir change

f730d0e

Add TS deps and build

5fb71f9

build folder, fix npm scripts and config

ce03313

Update tsconfig.json

7fb47e7

Move to jest

9655030

fix imports

df270ee

add prepublish script

23a80b0

Add coverage, remove types for now

a40f6c6

Add ts-node

b9c3bd0

Remove unnecessary catch clause

89da99b

move ts deps to devDependencies

f78dad1

Revert homedir change

36e4713

Merge branch 'typescript-build-deps' of github.com:shekharkhedekar/ft…

8edd845

…p-deploy into typescript-build-deps

revert test catch removal

0251dd6

shekharkhedekar commented Aug 10, 2022

View reviewed changes

simonh1000 reviewed May 13, 2023

View reviewed changes

playground/test_script.js

Copy link

Owner

simonh1000 May 13, 2023

Choose a reason for hiding this comment

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

please don't remove this file

simonh1000 reviewed May 13, 2023

View reviewed changes

simonh1000 requested changes May 13, 2023

View reviewed changes

shekharkhedekar closed this Jan 13, 2024

Add Typescript deps and build #148

Add Typescript deps and build #148

Uh oh!

Conversation

shekharkhedekar commented May 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Checks

Uh oh!

simonh1000 commented May 2, 2022

Uh oh!

shekharkhedekar commented May 2, 2022

Uh oh!

shekharkhedekar commented May 6, 2022

Uh oh!

simonh1000 commented May 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonh1000 commented May 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonh1000 commented May 7, 2022

Uh oh!

shekharkhedekar commented May 8, 2022

Uh oh!

shekharkhedekar commented May 8, 2022

Uh oh!

simonh1000 commented May 9, 2022

Uh oh!

shekharkhedekar commented May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shekharkhedekar commented May 30, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shekharkhedekar commented Aug 10, 2022

Uh oh!

simonh1000 commented Aug 18, 2022

Uh oh!

DEPSTRCZ commented May 11, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonh1000 left a comment

Choose a reason for hiding this comment

Uh oh!

shekharkhedekar commented Jan 13, 2024

Uh oh!

Reviewers

shekharkhedekar commented May 1, 2022 •

edited

Loading

shekharkhedekar commented May 9, 2022 •

edited

Loading