Renamed pathToJest to jestCommandLine in ProjectWorkspace #45

rossknudsen · 2020-05-05T11:45:19Z

Should close #44

rossknudsen · 2020-05-05T21:05:56Z

I've updated the changelog, bumping the major version since there a breaking change in the public API.

connectdotz

@rossknudsen sorry took a while to respond, thanks for making this change! 👍

Before we fully migrate to ts, still need to maintain index.d.ts by hand, please make sure to update that also.

Had some questions below, let's discuss...

connectdotz · 2020-05-16T20:57:40Z

src/project_workspace.ts

-    pathToJest: string,
-    pathToConfig: string,
+    jestCommandLine: string,
    localJestMajorVersion: number,
+    pathToConfig?: string,


if we agree on the shadow access approach, then we definitely do not want to change the order of these arguments...

I remember why I wanted to change the order of these arguments. Its because the pathToConfig property is genuinely optional. It is checked before it is used and it makes sense that there are occasions when there is no config file - just use the Jest defaults. For the purposes of this PR, I have reverted the constructor to maintain backwards compatibility. I will probably aim to tackle this scenario in the TS conversion PR. I personally think that we should use an interface for this - which is easier to do in TS.

To work around this issue in my code I do this: pathToConfig: undefined as unknown as string.

Just a thought -- If we're having to worry about argument order, that signals to me we should consider using an Object.

Yes, I agree that an object would be the best option here. But my opinion is that it will be easier to make that transition once more of the code is converted to Typescript. I just played with defining an interface for the ProjectWorkspace class, and then realized that I would need to update some existing code in JS with Flow typings...

actually come to think about this, why is this a class? what does it do other than holding some config data? maybe in the next major version, possible typescript conversion (?), we can rewrite this...

Sorry to derail that thread. I've created #46 to capture that idea. We can defer that for now.

To get back to the original point, the API is messy. I'd say leave the argument order for now and defer changing the API signature. In the interim Ross, you could build a factory function that only has to add that pathToConfig: undefined as unknown as string mess once.

const workspace = ({ ... }) => new ProjectWorkspace( ... );

connectdotz · 2020-05-16T21:12:23Z

src/project_workspace.ts

+   *
+   * @type {string?}
+   */
+  pathToJest?: string;


instead of maintaining both jestCommandLine and pathToJest internally and separately, what do you think if we just have shadow accessors (getter/setter) for pathToJest that backed by the one and only jestCommandLine property in the ProjectWorkspace. All codes/types can then just move to adopt the new name without checking both properties...

Yeah that sounds good. Getters and setters didn't occur to me for use in JS. I think it will solve this problem, maintaining the constructor as is. Will have a look at it.

you can even consider adding some warning messages in the getter/setter to remind people to migrate...

Do you mean console.warn(...)?

connectdotz

thanks for the update, a few comments below.

I think basically we are trying to push for runtime backward compatibility (for our client) while transitioning over to the new name. In order to minimize the complexity of checking both properties within this package, we adopted the shadow accessors so the rest of the system can safely focus only on the new property, i.e. none of the classes/functions outside of ProjectWorkspace should consider pathToJest...

connectdotz · 2020-05-17T14:50:58Z

src/Process.js

@@ -18,7 +18,7 @@ import ProjectWorkspace from './project_workspace';
 */
 // eslint-disable-next-line import/prefer-default-export
 export const createProcess = (workspace: ProjectWorkspace, args: Array<string>): ChildProcess => {
-  const runtimeExecutable = [workspace.pathToJest, ...args];
+  const runtimeExecutable = [workspace.jestCommandLine || workspace.pathToJest, ...args];


do we still need to check pathToJest here?

I was kind of forward thinking here, that if we were to replace the workspace config with an object, then the pathToJest might have been supplied. But it does come back to timing, maybe the pathToJest will be removed then. Happy to remove it.

Actually in the light of the rest of this conversation, I'll remove this. If/when we move to a config object, then we might need to check both fields - depending on how long we want to maintain backwards compatibility.

connectdotz · 2020-05-17T14:53:16Z

src/__tests__/project_workspace.test.js

@@ -11,9 +11,9 @@ import ProjectWorkspace from '../project_workspace';

 describe('setup', () => {


should we add a test to ensure that pathTojest is just a shadow accessor of jestCommandLine and not its own property?

That sounds like it might be fragile if we end up testing implementation details. Can we do that without knowing the internals?

Given a new ProjectWorkspace created with a jestCommandLine:

it should return the jestCommandLine
const commandLine = {} /* some arg */ const instance = new ProjectWorkspace(..., commandLine, ...); const expected = commandLine; const actual = instance.jestCommandLine; expect(actual).toBe(expected);

it should continue to support the deprecated pathToJest
// The same setup with const actual = instance.jestCommandLine; expect(actual).toBe(expected);

Given a new ProjectWorkspace created with a pathToJest:

it should return the jestCommandLine

it should continue to support the deprecated pathToJest

I had a test which exercised this functionality (at least IMO). I moved it from the settings tests to this file. Here is the definition:

it('ensure that pathToJest is a shadow accessor for jestCommandLine', () => { const localJestMajorVersion = 1000; const pathToConfig = 'test'; const jestCommandLine = 'path_to_jest'; const rootPath = 'root_path'; const workspace = new ProjectWorkspace(rootPath, jestCommandLine, pathToConfig, localJestMajorVersion); // first check that both pathToConfig and jestCommandLine are the same value. expect(workspace.jestCommandLine).toBe(jestCommandLine); expect(workspace.pathToJest).toBe(jestCommandLine); // then update the pathToJest property. const updatedPathToJest = 'updated pathToJest'; workspace.pathToJest = updatedPathToJest; // check that both pathToConfig and jestCommandLine yield the same value. expect(workspace.jestCommandLine).toBe(updatedPathToJest); expect(workspace.pathToJest).toBe(updatedPathToJest); });

Note that it is not possible to 'create' a config object with pathToJest rather than jestCommandLine because the constructor does not allow it (we should test this if/when we have a config object). To test the shadow access, I have used property assignment instead. Its unlikely anyone would create the object and then reassign the properties but it does prove the desired implementation.

connectdotz · 2020-05-17T14:58:09Z

src/Process.js

@@ -18,7 +18,7 @@ import ProjectWorkspace from './project_workspace';
 */
 // eslint-disable-next-line import/prefer-default-export
 export const createProcess = (workspace: ProjectWorkspace, args: Array<string>): ChildProcess => {
-  const runtimeExecutable = [workspace.pathToJest, ...args];
+  const runtimeExecutable = [workspace.jestCommandLine || workspace.pathToJest, ...args];


do we still need to check pathToJest here? The whole idea of using shadow accessor is so that the rest of the system doesn't need to concern itself with pathToJest anymore, as far as they concern, everything has changed to jestCommandLine...

connectdotz · 2020-05-17T15:02:39Z

src/__tests__/process.test.js

-  it('spawns a command with spaces from workspace.pathToJest', () => {
-    const workspace: any = {pathToJest: 'npm test --'};
+  it('spawns the command from workspace.jestCommandLine ignoring workspace.pathToJest', () => {
+    const workspace: any = {jestCommandLine: 'jest', pathToJest: 'not jest'};


this is not a valid workspace, we should not have pathToJest property in workspace any more...

connectdotz · 2020-05-17T15:06:17Z

src/__tests__/project_workspace.test.js

@@ -11,9 +11,9 @@ import ProjectWorkspace from '../project_workspace';

 describe('setup', () => {


this is the place I think we should test pathToJest is a shadow accessor instead of a real concrete property, while all the other classes should only deal with jestCommandLine.

connectdotz · 2020-05-17T15:12:49Z

index.d.ts

    outputFileSuffix?: string,
    collectCoverage?: boolean,
    debug?: boolean,
  );
+  jestCommandLine: string;
  pathToJest: string;


given this is a type file that it won't impact runtime (the binary out there that uses pathToJest will continue to work), we could get rid of pathToJest, so our customers will see ts warning during development and will need to fix the signature in their next release.

connectdotz

thanks for the update, see comments below

connectdotz · 2020-05-26T18:45:02Z

tsconfig.json

@@ -3,7 +3,7 @@
      /* Basic Options */
      "target": "es6",                          /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017', 'ES2018', 'ES2019' or 'ESNEXT'. */
      "module": "commonjs",                     /* Specify module code generation: 'none', 'commonjs', 'amd', 'system', 'umd', 'es2015', or 'ESNext'. */
-      "lib": ["ES2015"],                        /* Specify library files to be included in the compilation. */
+      "lib": ["DOM", "ES2015"],                 /* Specify library files to be included in the compilation. */


noticed you added 'DOM' here, is this intentional?

The console global is defined in the DOM lib. I'm using the console to emit warnings when using the deprecated properties.

I see... based on tsconfig doc:

Note: If --lib is not specified a default list of libraries are injected. The default libraries injected are:
► For --target ES5: DOM,ES5,ScriptHost
► For --target ES6: DOM,ES6,DOM.Iterable,ScriptHost

seems like target es6 by default already include DOM, in addition to es2015, maybe we don't need to specify lib at all?

Anyway, it's not wrong just a bit redundant...

I'm fine with that. Just commented the line to match the rest of the config file. We'll consolidate that config once everything is converted to TS.

connectdotz · 2020-05-26T18:49:29Z

src/project_workspace.ts

@@ -6,6 +6,16 @@
 *
 */

+export interface ProjectWorkspaceConfig {
+  jestCommandLine: string;
+  pathToConfig?: string;


I hate to mention this now, but if we are going for jestCommandLine, do you think we should also retire pathToConfig? Sorry, I should have thought about this earlier...

IMO I think the pathToConfig property is still needed. It just needs to be optional rather than required. Since I was introducing the ProjectWorkspaceConfig interface, which hopefully will be the future, I thought I'd mark pathToConfig as optional from the get go.

I am not sure what the use case will be... but I am fine in leaving it in for now.

…onfiguration.

… and pathToJest combine

merge

connectdotz

just a few minor clean up, we are ready to merge once done.

connectdotz · 2020-05-31T00:56:08Z

CHANGELOG.md

@@ -7,6 +7,9 @@ Bug-fixes within the same version aren't needed
 * Replace babylon and typescript parsers with @babel/parser 7.x - @firsttris 
 -->

+### 27.3.0


actually no need to specify the version number, just add the change in the commented out master section above. When we are ready to cut the release, they will be moved to the proper version then...

connectdotz · 2020-05-31T00:59:54Z

tsconfig.json

+      // "lib": ["DOM", "ES2015"],              /* Specify library files to be included in the compilation. */
      "allowJs": false,                         /* Allow javascript files to be compiled. */
      // "checkJs": true,                       /* Report errors in .js files. */
      // "jsx": "preserve",                     /* Specify JSX code generation: 'preserve', 'react-native', or 'react'. */


please remove the commented out statements

connectdotz · 2020-05-31T01:02:41Z

src/project_workspace.ts

@@ -6,6 +6,16 @@
 *
 */

+export interface ProjectWorkspaceConfig {
+  jestCommandLine: string;
+  pathToConfig?: string;


I am not sure what the use case will be... but I am fine in leaving it in for now.

connectdotz · 2020-06-06T15:34:49Z

@rossknudsen thanks for the PR 👍 , I will cut a beta soon so we can start to play with the new changes

rossknudsen · 2020-06-07T00:12:04Z

Sounds good, I was starting to see some parsing issues around it.each, so I'm keen to see how the new one works. I'll update my other TS PR too - whether it makes this release or the next one.

connectdotz reviewed May 16, 2020

View reviewed changes

rossknudsen force-pushed the feature/rename-path-to-jest branch from da950bf to f879e56 Compare May 17, 2020 10:42

connectdotz reviewed May 17, 2020

View reviewed changes

seanpoulter mentioned this pull request May 17, 2020

Improvement: consider changing the API for new ProjectWorkspace #46

Open

rossknudsen force-pushed the feature/rename-path-to-jest branch from ff6738e to 92d6696 Compare May 18, 2020 09:33

connectdotz reviewed May 26, 2020

View reviewed changes

Ross added 8 commits May 30, 2020 17:39

refactor: Renamed pathToJest to jestCommandLine in ProjectWorkspace c…

c92b58a

…onfiguration.

chore: updated Changelog.

886fb83

fix: reverted ProjectWorkspace constructor and linked jestCommandLine…

95f4f56

… and pathToJest combine

fix: unit tests for previous commit.

6a960bb

fix: chained pathToJest to jestCommandLine in ProjectWorkspace.

c0486cb

test: added unit test to maintain coverage.

19528f3

merge

refactor: added factory method to build ProjectWorkspace.

13d4b24

fix: Added console warning on accessing deprecated pathToJest.

bf100dc

merge

rossknudsen force-pushed the feature/rename-path-to-jest branch from 92d6696 to bf100dc Compare May 30, 2020 05:42

Ross Knudsen added 2 commits May 30, 2020 18:16

build: removed redundant TS config option.

0e2f163

chore: updated version in package.json to match changelog.

f0aff2c

connectdotz approved these changes May 31, 2020

View reviewed changes

build: Removed redundant comments from TS config.

9afad42

connectdotz merged commit a1f4f36 into jest-community:master Jun 6, 2020

rossknudsen deleted the feature/rename-path-to-jest branch June 7, 2020 09:52

		@@ -11,9 +11,9 @@ import ProjectWorkspace from '../project_workspace';

		describe('setup', () => {

Renamed pathToJest to jestCommandLine in ProjectWorkspace #45

Renamed pathToJest to jestCommandLine in ProjectWorkspace #45

Conversation

rossknudsen commented May 5, 2020 • edited Loading

rossknudsen commented May 5, 2020

connectdotz 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connectdotz 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

connectdotz May 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connectdotz 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

connectdotz 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

connectdotz commented Jun 6, 2020

rossknudsen commented Jun 7, 2020 • edited Loading

rossknudsen commented May 5, 2020 •

edited

Loading

connectdotz May 17, 2020 •

edited

Loading

rossknudsen commented Jun 7, 2020 •

edited

Loading