(Archived) Adopt TypeScript #1497

artem-zakharchenko · 2019-09-16T11:16:46Z

🚀 Why this change?

Adds Prettier
Introduces basic TypeScript stack to Dredd
Adjusts source code and tests to use ES6 syntax
Provides a minimum of types annotations to basic utility functions

📝 Related issues and Pull Requests

Relates to Future Dredd proposal #1186

✅ What didn't I forget?

To write docs (not applicable, except contribution docs)
To write tests
To put Conventional Changelog prefixes in front of all my commits and run npm run lint
Adjust CircleCI
Adjust AppVeyor
Remove Rollup and related dependencies, because the build is performed by TypeScript compiler

Preparation for #1497

artem-zakharchenko · 2019-09-16T11:54:55Z

.eslintrc.js

    // This is to allow a convention for exporting functions solely for
    // the purpose of the unit tests, see
    // https://github.com/apiaryio/dredd-transactions/pull/179#discussion_r206852270
    'no-underscore-dangle': 'off',

+    'import/prefer-default-export': 'off',


Allows export const a = 'anything' exports from files.

Makes internals easier to export for testing

Doesn't pollute the default export

artem-zakharchenko · 2019-09-24T13:59:16Z

CircleCI build is passing. Switching to configure AppVeyor to include build step in the CI.

artem-zakharchenko · 2019-09-24T14:30:12Z

@kylef, could you please review?

Mainly CI configs and build setup, but I'd be thankful for a few files I've migrated to TypeScript too. Note that as of now Dredd won't emit any type definitions because that's incompatible with allowJs option that enables js/ts codebase. We would need to rewrite all existing modules to TypeScript for it to emit definition files, it seems.

kylef

Filing initial review, I have not look at the .ts and .js files yet in the diff and only the surrounding configuration changes. Working on that next but I think it will take a bit of time and I don't think I'll get it done today.

For future, i'd consider breaking down the PR into separate sections:

Prettifier
- Initial PR could have little diff if there was prettifier config to make existing code style, then you could incrementally make separate PRs changing rules to their defaults.
TypeScript

.circleci/config.yml

.eslintrc.js

kylef

Reviewed up until lib/configUtils.js

lib/CLI.js

lib/Dredd.js

kylef · 2019-09-27T08:37:06Z

lib/Hooks.js

+    this.beforeEachValidation = this.beforeEachValidation.bind(this)
+    this.afterEach = this.afterEach.bind(this)
+    this.log = this.log.bind(this)
+    ;({ logs: this.logs, logger: this.logger } = options)


Doesn't seem like the semi-colons are removed in the other files I've reviewed so far, why are they removed from other lines in this file and inserted at the start of this line instead of the end of the prior line?

I will align semicolons consistency and remove them from the source code altogether.

Because ({ logs: this.logs, logger: this.logger } = options) is not a valid expression per formatting tools, so it appends an explicit semicolon in no-semi mode to let it be evaluated properly (and not the part of the previous expression).

That's black sorcery. Can we replace all expressions starting with a semicolon with some boring JavaScript code?

this.logs = options.logs this.logger = options.logger

Yes, sure. Moreover, we can replace them in the original code as one of the first PRs:

dredd/lib/Hooks.js

Line 18 in 91ceef1

({ logs: this.logs, logger: this.logger } = options);

lib/childProcess.js

honzajavorek

I'm unable to review the PR. I went through tens of files, but it's so tedious I give up. Even the GitHub UI is responding slowly. This needs to be broken down into smaller PRs and rolled out incrementally.

E.g. there are codebase-wide changes, such as removing the semicolons. Such change can be perfectly done as a single PR titled "Remove semicolons from the codebase", where all content is automated and cognitively easy-to-review (despite the huge amount of changes made). Having that as a separate change removes tens of files from the diffs on any subsequent PR representing the TS migration and makes reviewing easier as I don't have to question my mind "is this line changed only because of a semicolon or does it contain additional refactoring?" 100% of time. I used semicolons as a blatant example, but I can see many parts of the changes in the PR, which can be separated out, issued as an easy-to-review PR, and released as an easy-to-fix-if-shit-happened increment. Such steps can be introducing of Prettier (without any manual edits, those sent as a separate commit or PR), change from requires to imports, etc. Especially manual refactorings made on the code should be sent separately as those should stand out and be reviewed carefully. The way it is now they're sunk in reformats made by Prettier. If some files are rewritten to TS and some are not, the rewritten ones should be sent as separate, incremental PRs.

I know I haven't been the best example in the past in issuing nice, small changes, but we need to start somewhere. I'll be the bad guy today and please, do revenge on my PRs in the future if they're too big.

honzajavorek · 2019-09-27T16:07:12Z

.circleci/config.yml

 jobs:
+  build:


Does it need to be tested separately if it's a part of all the other jobs?

Besides, this job isn't included in the workflow, so it isn't currently being ran.

honzajavorek · 2019-09-27T16:09:21Z

.circleci/config.yml

    image: circleci/python:3.6-node

  - &test-steps
    steps:
      - checkout
      - npm-install
+      - run: npm run build


Should the build be an explicit step or should it be implicitly included as something like pretest script?

I've tried to perform a build once and store it, but since its quite fast, we've decided with @kylef that such setup may be an overkill (attaching a workspace with the build takes multiple times longer than the build itself). I'm okay to revisit this, if you find it necessary.

honzajavorek · 2019-09-27T16:12:04Z

.npmignore

@@ -0,0 +1,2 @@
+# Prevent publishing source code to NPM
+lib


Use package.json's files, please - https://medium.com/@jdxcode/for-the-love-of-god-dont-use-npmignore-f93c08909d8d

honzajavorek · 2019-09-27T16:31:09Z

lib/Dredd.js

-  });
-}
+  if (typeof options === 'function') {
+    ;[options, callback] = [{}, options]


What's the purpose of the semicolon here?

To support initially invalid JavaScript syntax. I kept an intention to do as less changes to the existing code base as possible.

honzajavorek · 2019-09-27T16:46:43Z

lib/Hooks.js

+    this.beforeEachValidation = this.beforeEachValidation.bind(this)
+    this.afterEach = this.afterEach.bind(this)
+    this.log = this.log.bind(this)
+    ;({ logs: this.logs, logger: this.logger } = options)


That's black sorcery. Can we replace all expressions starting with a semicolon with some boring JavaScript code?

this.logs = options.logs this.logger = options.logger

artem-zakharchenko · 2019-09-30T08:46:11Z

Thanks for attempts, @honzajavorek. I understand your pain and would like to address it. I'm fine with splitting this PR into multiple, but you need to be aware of certain implications, and that some steps of these changes cannot be rolled out separately.

"is this line changed only because of a semicolon or does it contain additional refactoring?"

Very easy: no, this pull request contains 0 refactoring done in JavaScript files. My bad if I haven't communicated this properly, I've commented this to one of the Kyle's points, but this should've been attached in the PR's description.

If some files are rewritten to TS and some are not, the rewritten ones should be sent as separate, incremental PRs.

Sounds like a good idea. I'm for it.

E.g. there are codebase-wide changes, such as removing the semicolons. Such change can be perfectly done as a single PR titled "Remove semicolons from the codebase"

Well, not really. Semicolons are preserved in the source code because it is the code run by Node. The older versions of Node cannot cope with semicolons' absence, so you cannot remove them from the source code unless you introduce a build step that would ensure them.

Pull requests pipeline

I can suggest the next sequence of Pull request to be made:

Fix syntactically invalid JavaScript in the existing source code (Fixes syntactically invalid JavaScript #1525)
Introduce code formatting (Adds Prettier #1526)
Introduce TypeScript (Adds TypeScript #1529)
- This inevitably includes mocha+ts, which IS a huge change
Re-write a few simple files with TypeScript

But beware that certain changes will be big, you cannot break down related changes (i.e. you can't introduce TS and change testing setup in two separate pull requests, the build simply won't pass).

honzajavorek · 2019-09-30T10:03:13Z

👍 makes sense. If we could have big PRs at least consisting of commits each containing one kind-of-atomic change (like semicolons), that would also make the review easier. This PR contained some back-and-forth changes in the commits, which discouraged me to go commit-by-commit.

artem-zakharchenko · 2019-09-30T10:04:32Z

@honzajavorek, yeah, that's true. It was a discovery road for me, so some things went back and forth. Next pull request will be cleaner based on the knowledge I've gather during this implementation.

honzajavorek · 2019-09-30T10:35:07Z

I completely understand, my PRs usually look very similar 🙂 When they grow big, I roll up my sleeves and dive into breaking them down and rebasing into something smaller.

When I was alone in the team, @kylef was always nice to never be too harsh about my big PRs, but he got me positively motivated by naturally procrastinating reviews on them. I knew that small PRs will get through faster 😄

artem-zakharchenko · 2019-10-03T12:15:13Z

Closed in favor of #1530

artem-zakharchenko added the in progress label Sep 16, 2019

honzajavorek added a commit that referenced this pull request Sep 16, 2019

chore: prepare deps for a transition to TypeScript

d764932

Preparation for #1497

honzajavorek mentioned this pull request Sep 16, 2019

Prepare deps for a transition to TypeScript #1498

Merged

3 tasks

artem-zakharchenko commented Sep 16, 2019

View reviewed changes

artem-zakharchenko force-pushed the 1186-future branch 6 times, most recently from 8d15e7a to 1682d97 Compare September 23, 2019 10:28

chore: adds typescript, rollup, mocha+ts

d7169da

artem-zakharchenko force-pushed the 1186-future branch from 1682d97 to 76d9aa5 Compare September 23, 2019 10:30

test: adjusts test suites imports to be compiled by typescript

722a50f

artem-zakharchenko force-pushed the 1186-future branch from 76d9aa5 to 722a50f Compare September 23, 2019 10:32

chore: points to "typings" in "package.json"

a363d2e

artem-zakharchenko force-pushed the 1186-future branch 11 times, most recently from 37cc40a to 3c2d7ad Compare September 23, 2019 12:49

chore: integrates build step to CI

b8b0304

artem-zakharchenko force-pushed the 1186-future branch from 3c2d7ad to b8b0304 Compare September 23, 2019 12:52

chore: resolves json imports compilation by typescript

9746627

artem-zakharchenko force-pushed the 1186-future branch from 07a5a9a to 9746627 Compare September 23, 2019 13:55

artem-zakharchenko force-pushed the 1186-future branch 2 times, most recently from 9164de5 to c94e583 Compare September 24, 2019 13:30

chore: builds Dredd before smoke tests

94522b9

artem-zakharchenko force-pushed the 1186-future branch from c94e583 to 94522b9 Compare September 24, 2019 13:51

chore: adds build step to AppVeyor CI

760d899

artem-zakharchenko changed the title ~~Adds TypeScript~~ Adopt TypeScript Sep 24, 2019

artem-zakharchenko requested a review from kylef September 24, 2019 14:29

chore: removes Rollup

9dc1dde

kylef reviewed Sep 25, 2019

View reviewed changes

.circleci/config.yml Outdated Show resolved Hide resolved

.eslintrc.js Show resolved Hide resolved

chore: removes unnecessary cache restoration in CircleCI

fd46b03

artem-zakharchenko removed the in progress label Sep 26, 2019

artem-zakharchenko requested a review from honzajavorek September 26, 2019 11:30

kylef reviewed Sep 27, 2019

View reviewed changes

artem-zakharchenko added 3 commits September 27, 2019 11:56

fix: uses function declaration for "childProcess" module

1b6ab77

fix: applies consistent file formatting to source code

b7911c8

fix: removes "logger" logging used for debugging

7b15907

honzajavorek suggested changes Sep 27, 2019

View reviewed changes

This was referenced Sep 30, 2019

Adopt TypeScript #1524

Open

Fixes syntactically invalid JavaScript #1525

Merged

artem-zakharchenko mentioned this pull request Oct 1, 2019

Adds TypeScript #1529

Merged

3 tasks

artem-zakharchenko changed the title ~~Adopt TypeScript~~ (Archived) Adopt TypeScript Oct 3, 2019

artem-zakharchenko closed this Oct 3, 2019

artem-zakharchenko deleted the 1186-future branch October 3, 2019 12:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(Archived) Adopt TypeScript #1497

(Archived) Adopt TypeScript #1497

artem-zakharchenko commented Sep 16, 2019 •

edited

Loading

artem-zakharchenko Sep 16, 2019

artem-zakharchenko commented Sep 24, 2019

artem-zakharchenko commented Sep 24, 2019

kylef left a comment

kylef left a comment

kylef Sep 27, 2019

artem-zakharchenko Sep 27, 2019

honzajavorek Sep 27, 2019

artem-zakharchenko Sep 30, 2019

honzajavorek left a comment

honzajavorek Sep 27, 2019

honzajavorek Sep 27, 2019

honzajavorek Sep 27, 2019

artem-zakharchenko Sep 30, 2019

honzajavorek Sep 27, 2019

honzajavorek Sep 27, 2019

artem-zakharchenko Sep 30, 2019

honzajavorek Sep 27, 2019

artem-zakharchenko commented Sep 30, 2019 •

edited

Loading

honzajavorek commented Sep 30, 2019

artem-zakharchenko commented Sep 30, 2019

honzajavorek commented Sep 30, 2019

artem-zakharchenko commented Oct 3, 2019

(Archived) Adopt TypeScript #1497

(Archived) Adopt TypeScript #1497

Conversation

artem-zakharchenko commented Sep 16, 2019 • edited Loading

🚀 Why this change?

📝 Related issues and Pull Requests

✅ What didn't I forget?

Choose a reason for hiding this comment

artem-zakharchenko commented Sep 24, 2019

artem-zakharchenko commented Sep 24, 2019

kylef left a comment

Choose a reason for hiding this comment

kylef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honzajavorek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artem-zakharchenko commented Sep 30, 2019 • edited Loading

Pull requests pipeline

honzajavorek commented Sep 30, 2019

artem-zakharchenko commented Sep 30, 2019

honzajavorek commented Sep 30, 2019

artem-zakharchenko commented Oct 3, 2019

artem-zakharchenko commented Sep 16, 2019 •

edited

Loading

artem-zakharchenko commented Sep 30, 2019 •

edited

Loading