make input paths relative to current working directory #20

brekk · 2017-10-03T17:16:42Z

Thanks for this library, it's exactly what I was looking for!

I was unable to run this library without augmenting it:

tscodeshift -t dist/src/transforms/mocha.js 'my-fixture-file.ts'

Always resulted in:

Failed to run tscodeshift. {
    Error: Cannot find module 'dist/src/transforms/mocha.js'
    [...]
}

node version: v8.5.0

I also added a test to prove that this works as expected.

Please let me know if there are additional things you'd like me to change.

KnisterPeter · 2017-10-03T17:32:43Z

Awesome. Thanks for the contribution. 🚀
After a first look, I would like to have the yarn file removed. I favor npm and even without yarn.lock it will be installable with yarn. The yarn.lock file will be too quickly out of date.

KnisterPeter · 2017-10-03T17:33:55Z

For relative paths you could also used 'tscodeshift -t ./dist/src/transforms/mocha.js 'my-fixture-file.ts'The./` in front of the relative path makes it a node file and not a node module.

KnisterPeter · 2017-10-03T17:37:36Z

And since you updated typescript to the latest version it my be required to update the test harness as well.
This is a manuall npm script and maybe a few parser updates to implement.
The harness is cached and only runs manually and on CI for performance reasons.

EDIT: Oh, don't mind. This would be located in the ts-emitter project. Not in here. So this is fine.

brekk · 2017-10-03T17:56:47Z

@KnisterPeter Cool, I'll remove the yarn.lock file. I could be wrong here but I believe I tried it with ./ relative pathing and had the same issue -- will verify.

KnisterPeter · 2017-10-03T21:25:03Z

package.json

@@ -13,7 +13,7 @@
    "start": "npm test -- --watch",
    "clean": "rimraf dist",
    "build": "tsc",
-    "test": "jest",
+    "test": "npm run build && jest",


Running build before jest is not required with ts-jest.

So, relative to how I am building stuff, this is; basically I wanted to be able to ask the compiled / for-publication version to build things at the command-line, and dist/src doesn't exist at that point.

More comments below.

KnisterPeter · 2017-10-03T21:25:35Z

package.json

@@ -59,13 +60,16 @@
    "transform": {
      ".(ts|tsx)": "<rootDir>/node_modules/ts-jest/preprocessor.js"
    },
-    "testRegex": "\\.test\\.(ts|tsx)$",
+    "testRegex": "\\.test\\.(ts|tsx|js)$",


Why do you need to find js files for tests?

See comment here

KnisterPeter · 2017-10-03T21:28:07Z

src/index.test.ts

@@ -123,3 +123,68 @@ it('convert var declarations to let declarations', () => {

  expect(actual).toBe(expected);
 });
+
+test(`api`, () => {


I'm not sure what you are testing here.

So, there's an else case that is very hard to get to inside of the ./transform#api.tscodeshift function, and this test covers that else case. Sorry, I should have clarified my test case after I got it working. (Basically the coverage was lowered by me adding the files I added, so by covering the else case, I was able to get coveralls to pass.)

Specifically this: https://travis-ci.org/KnisterPeter/tscodeshift/builds/237974069#L217

KnisterPeter · 2017-10-03T21:30:59Z

relative-paths.test.js

@@ -0,0 +1,42 @@
+/* global test, expect, afterEach */


This files belongs into the src folder. Even tests are sources.

So, in general the reason I have this .js file is more from my lack of familiarity with TypeScript. I am more than happy to have this file be in src, but relative to the problem I was trying to solve, I didn't wanna jump through hoops of TypeScript fixing while trying to get the gist of this thing working. I think specifically I was running into some issues trying to use path -- this evening I'll try and regroup and clarify my intentions in this PR.

brekk · 2017-10-03T23:41:00Z

src/transform.ts

-const api = {
-  tscodeshift: (source: string|ts.Node) => {
+export const api = {
+  tscodeshift: (source: string|ts.Node): Collection<any, any> => {


@KnisterPeter I had to make this change in order to get tsc to compile properly -- I'm guessing there's a better way to do this, but I was running into some problems locally.

brekk · 2017-10-03T23:41:52Z

relative-paths.test.js

+const execa = require('execa')
+const path = require('path')
+
+const CLI = path.resolve(__dirname, `./dist/src/index.js`)


@KnisterPeter This is the reason for that additional build step -- I want to be able to reference the compiled version of this file in the test.

KnisterPeter · 2017-10-04T17:46:53Z

src/index.test.ts

@@ -124,7 +124,7 @@ it('convert var declarations to let declarations', () => {
  expect(actual).toBe(expected);
 });

-test(`api`, () => {
+test(`api.tscodeshift can process raw ts.Nodes`, () => {
  expect(typeof api).toBe(`object`);
  expect(typeof api.tscodeshift).toBe(`function`);
  const removeCircularCrap = (x: any): any => {


Rename this from 'Crap' to 'References' 😉

KnisterPeter · 2017-10-04T17:47:16Z

src/index.test.ts

@@ -136,7 +136,6 @@ test(`api`, () => {
    return x;
  };
  const expected = JSON.parse(JSON.stringify(removeCircularCrap(api.tscodeshift(`false`))));
-  // expect(JSON.parse(JSON.stringify(removeCircularCrap(expected)))).toEqual(circular)


Remove commented out lines

KnisterPeter · 2017-10-04T17:50:23Z

src/relative-paths.test.ts

@@ -0,0 +1,50 @@
+/* global test, expect, afterEach */
+// https://stackoverflow.com/questions/39415661


You do not need this reference for the import-require statement.
Its part of the official typescript documentation. No need to quote these.

https://www.typescriptlang.org/docs/handbook/modules.html

KnisterPeter · 2017-10-04T17:51:38Z

Besides all my comments. Thanks for you awesome contribution. 👍

KnisterPeter · 2017-10-05T06:12:52Z

I've updated your test code to do not require npm run build before jest.
This means npm run jest -- --watch is possible again.

fix: make paths relative

7bd544b

brekk force-pushed the bug/relative-path branch from fc626b1 to 7bd544b Compare October 3, 2017 20:27

brekk added 2 commits October 3, 2017 13:56

fix shadowed path variable

b3a74ad

fix other shadowed path variable

9734c7d

brekk force-pushed the bug/relative-path branch from 10edbd8 to 9734c7d Compare October 3, 2017 21:23

KnisterPeter reviewed Oct 3, 2017

View reviewed changes

brekk commented Oct 3, 2017

View reviewed changes

brekk added 3 commits October 4, 2017 10:19

moved test

51819aa

clarify test intent

c78650f

remove js extension

e6511c0

KnisterPeter reviewed Oct 4, 2017

View reviewed changes

address comments

b77394c

KnisterPeter merged commit 218f9b8 into KnisterPeter:master Oct 5, 2017

KnisterPeter mentioned this pull request Oct 5, 2017

test: simplify test #21

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make input paths relative to current working directory #20

make input paths relative to current working directory #20

brekk commented Oct 3, 2017 •

edited

KnisterPeter commented Oct 3, 2017

KnisterPeter commented Oct 3, 2017

KnisterPeter commented Oct 3, 2017 •

edited

brekk commented Oct 3, 2017

KnisterPeter Oct 3, 2017

brekk Oct 3, 2017

KnisterPeter Oct 3, 2017

brekk Oct 3, 2017

KnisterPeter Oct 3, 2017

brekk Oct 3, 2017

brekk Oct 4, 2017

KnisterPeter Oct 3, 2017

brekk Oct 3, 2017

brekk Oct 3, 2017

brekk Oct 3, 2017

KnisterPeter Oct 4, 2017

KnisterPeter Oct 4, 2017

KnisterPeter Oct 4, 2017

KnisterPeter commented Oct 4, 2017

KnisterPeter commented Oct 5, 2017

		@@ -0,0 +1,50 @@
		/* global test, expect, afterEach */
		// https://stackoverflow.com/questions/39415661

make input paths relative to current working directory #20

make input paths relative to current working directory #20

Conversation

brekk commented Oct 3, 2017 • edited

KnisterPeter commented Oct 3, 2017

KnisterPeter commented Oct 3, 2017

KnisterPeter commented Oct 3, 2017 • edited

brekk commented Oct 3, 2017

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

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

KnisterPeter commented Oct 4, 2017

KnisterPeter commented Oct 5, 2017

brekk commented Oct 3, 2017 •

edited

KnisterPeter commented Oct 3, 2017 •

edited