Load custom formatter by relative path only if it begins with a dot #1413

dawn-minion · 2020-08-26T08:00:21Z

This commit makes it possible to load a custom formatter that is not relative to the current working directory, such as one installed
globally, or via Yarn PNP. This change makes it so we only load by a relative path if it begins with a dot, otherwise it will load
it as a regular non-relative require.

Fixes #1412

davidjgoss · 2020-10-05T23:25:16Z

Would we consider making this more explicit, maybe borrowing webpack's tilda syntax:

require a node module

$ cucumber-js --format "~@my-scope/my-formatter-package"

Normal (current) behaviour:

$ cucumber-js --format "lib/formatters/local-custom-formatter.js"

(Let me know if I've missed why this wouldn't work!)

charlierudolph · 2020-10-10T16:52:39Z

@davidjgoss I don't think this is about a node module vs not. Global node modules cannot be required regularly and local node modules should be fine with being referenced relative to the current path.

@dawn-minion I don't understand why this is needed. The current implementation should work fine with absolute paths - which is what I assume you are using for globally installed modules. I'll comment on the issue as I want more specifics about the current problem you're experiencing

charlierudolph · 2020-10-10T17:11:54Z

Okay plug in play is very interesting. Let's discuss this more in the issue.

dawn-minion · 2020-10-28T15:55:45Z

@charlierudolph Sorry for the delay, been busy with other things. I believe that should be it here.

charlierudolph · 2020-10-31T23:00:23Z

Code changes look good. Can you please add a note to the changelog under breaking changes?

This commit makes it possible to load a custom formatter that is not relative to the current working directory, such as one installed globally, or via Yarn PNP. If the path begins with a . it will be loaded relative to the current directory.

dawn-minion · 2020-11-02T09:55:57Z

@charlierudolph Should be ready now - had to remove the relative path parser from world.ts as well, as it would strip the leading ./ from the tests. All tests should be passing now

merceyz · 2020-11-15T23:36:24Z

src/formatter/builder.ts

+      const fullCustomFormatterPath = path.resolve(cwd, customFormatterPath)
+      CustomFormatter = require(fullCustomFormatterPath) // eslint-disable-line @typescript-eslint/no-var-requires
+    } else {
+      CustomFormatter = require(customFormatterPath) // eslint-disable-line @typescript-eslint/no-var-requires


This should be using createRequire to run the require from the cwd

Yeah that seems a fair point, although per the docs:

module.createRequire(filename)#

Added in: v12.2.0

Given we're still supporting Node 10 for the time being (its EOL is April 2021), could use https://github.com/sindresorhus/import-cwd instead.

I'd suggest using https://yarnpkg.com/package/create-require as it will use the node builtins when it can, so createRequire on node >= 12.2 createRequireFromPath on node >= 10.12, then a polyfil for the rest which is much better

Should actually use it for both branches here, which simplifies the logic and has the same end result

Added the above, works great for both cases, thanks for pointing it out

davidjgoss

LGTM, a couple of suggestions.

davidjgoss · 2020-11-24T23:06:54Z

src/formatter/builder.ts

+      const fullCustomFormatterPath = path.resolve(cwd, customFormatterPath)
+      CustomFormatter = require(fullCustomFormatterPath) // eslint-disable-line @typescript-eslint/no-var-requires
+    } else {
+      CustomFormatter = require(customFormatterPath) // eslint-disable-line @typescript-eslint/no-var-requires


Yeah that seems a fair point, although per the docs:

module.createRequire(filename)#

Added in: v12.2.0

Given we're still supporting Node 10 for the time being (its EOL is April 2021), could use https://github.com/sindresorhus/import-cwd instead.

CHANGELOG.md

merceyz · 2020-11-25T00:19:27Z

src/formatter/builder.ts

+    if (customFormatterPath.startsWith('.')) {
+      const fullCustomFormatterPath = path.resolve(cwd, customFormatterPath)
+      CustomFormatter = require(fullCustomFormatterPath) // eslint-disable-line @typescript-eslint/no-var-requires
+    } else {
+      CustomFormatter = require(customFormatterPath) // eslint-disable-line @typescript-eslint/no-var-requires
+    }
+


Adding create-require to the dependencies and importing it simplifies this down to

Suggested change

if (customFormatterPath.startsWith('.')) {

const fullCustomFormatterPath = path.resolve(cwd, customFormatterPath)

CustomFormatter = require(fullCustomFormatterPath) // eslint-disable-line @typescript-eslint/no-var-requires

} else {

CustomFormatter = require(customFormatterPath) // eslint-disable-line @typescript-eslint/no-var-requires

}

CustomFormatter = createRequire(cwd)(customFormatterPath)

Switched to this

dawn-minion · 2020-11-25T08:06:23Z

Sorry for the delay here, been a bit busy with a move. I've added a note about Yarn PnP support to the changelog, added create-require here as @merceyz suggested which simplifies the code. Ready for review again.

merceyz · 2020-11-25T08:11:04Z

src/formatter/builder.ts

+    let CustomFormatter = null
+
+    CustomFormatter = createRequire(cwd)(customFormatterPath)


Suggested change

let CustomFormatter = null

CustomFormatter = createRequire(cwd)(customFormatterPath)

const CustomFormatter = createRequire(cwd)(customFormatterPath)

merceyz · 2020-11-25T08:11:48Z

CHANGELOG.md

@@ -56,6 +57,7 @@ If anything is missing from the migration guide, please submit an issue.
  * Custom formatters will need to migrate
  * `json` formatter is deprecated and will be removed in next major release. Custom formatters should migrate to use the `message` formatter, or the [standalone JSON formatter](https://github.com/cucumber/cucumber/tree/master/json-formatter) as a stopgap.
 * Remove long-deprecated `typeName` from options object for `defineParameterType` in favour of `name`
+* Custom formatters are no longer loaded relative to the current directory unless it begins with a dot (e.g. `--format=./relpath/to/formatter`). 


NIT: They are always loaded relative to the cwd now

Right, the note being that it uses the regular require search path relative to cwd now, instead of always trying to load a file by path from the cwd

Hopefully that's a bit clearer now.

tsconfig.json

dawn-minion · 2020-11-26T14:56:12Z

Can't seem to reproduce the test failure on appveyor, it's passing locally here with node v10.23.0

davidjgoss

@dawn-minion I think the CI failure is an (unrelated) unstable test per #1457.

dawn-minion requested a review from charlierudolph August 26, 2020 08:00

dawn-minion force-pushed the issue_1412_formatter_path branch from 7347adc to 6d01071 Compare August 26, 2020 08:03

charlierudolph changed the title ~~#1412 Try loading formatter by relative dir then by name~~ Try loading formatter by relative dir then by name Oct 10, 2020

dawn-minion force-pushed the issue_1412_formatter_path branch from 6d01071 to ac93d12 Compare October 28, 2020 15:54

dawn-minion changed the title ~~Try loading formatter by relative dir then by name~~ Load custom formatter by relative path only if it begins with a dot Oct 28, 2020

dawn-minion added 2 commits November 2, 2020 10:54

Add note about this change to CHANGELOG breaking changes

4bb5a34

dawn-minion force-pushed the issue_1412_formatter_path branch from bfcac12 to 4bb5a34 Compare November 2, 2020 09:55

merceyz reviewed Nov 15, 2020

View reviewed changes

davidjgoss mentioned this pull request Nov 24, 2020

7.0.0 candidates #1290

Closed

5 tasks

davidjgoss added this to the 7.0.0 milestone Nov 24, 2020

davidjgoss reviewed Nov 24, 2020

View reviewed changes

merceyz reviewed Nov 25, 2020

View reviewed changes

Add YarnPNP to feature changelog, switch to create-require

91dbe2d

merceyz reviewed Nov 25, 2020

View reviewed changes

dawn-minion added 2 commits November 25, 2020 09:32

Change wording of custom formatter path change

54f2c6a

Make CustomFormatter const

47d0a8c

davidjgoss reviewed Nov 26, 2020

View reviewed changes

tsconfig.json Outdated Show resolved Hide resolved

merceyz mentioned this pull request Nov 26, 2020

fix(types): explicitly import the URL type unjs/create-require#3

Merged

Bump create-require to 1.1.1 and remove dom from tsconfig

36b2823

davidjgoss approved these changes Nov 26, 2020

View reviewed changes

davidjgoss merged commit ff56df4 into cucumber:master Nov 26, 2020

davidjgoss mentioned this pull request Apr 7, 2021

cli: esm support #1589

Merged

9 tasks

charlierudolph mentioned this pull request Apr 13, 2021

Load custom snippet syntax by relative path only if it begins with a dot #1640

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load custom formatter by relative path only if it begins with a dot #1413

Load custom formatter by relative path only if it begins with a dot #1413

dawn-minion commented Aug 26, 2020 •

edited

Loading

davidjgoss commented Oct 5, 2020 •

edited

Loading

charlierudolph commented Oct 10, 2020 •

edited

Loading

charlierudolph commented Oct 10, 2020

dawn-minion commented Oct 28, 2020

charlierudolph commented Oct 31, 2020

dawn-minion commented Nov 2, 2020

merceyz Nov 15, 2020

davidjgoss Nov 24, 2020 •

edited

Loading

merceyz Nov 25, 2020 •

edited

Loading

merceyz Nov 25, 2020

dawn-minion Nov 25, 2020

davidjgoss left a comment

davidjgoss Nov 24, 2020 •

edited

Loading

merceyz Nov 25, 2020 •

edited

Loading

dawn-minion Nov 25, 2020

dawn-minion commented Nov 25, 2020

merceyz Nov 25, 2020

dawn-minion Nov 25, 2020

merceyz Nov 25, 2020

dawn-minion Nov 25, 2020

dawn-minion Nov 25, 2020

dawn-minion commented Nov 26, 2020

davidjgoss left a comment

		let CustomFormatter = null

		CustomFormatter = createRequire(cwd)(customFormatterPath)

Load custom formatter by relative path only if it begins with a dot #1413

Load custom formatter by relative path only if it begins with a dot #1413

Conversation

dawn-minion commented Aug 26, 2020 • edited Loading

davidjgoss commented Oct 5, 2020 • edited Loading

charlierudolph commented Oct 10, 2020 • edited Loading

charlierudolph commented Oct 10, 2020

dawn-minion commented Oct 28, 2020

charlierudolph commented Oct 31, 2020

dawn-minion commented Nov 2, 2020

Choose a reason for hiding this comment

davidjgoss Nov 24, 2020 • edited Loading

Choose a reason for hiding this comment

module.createRequire(filename)#

merceyz Nov 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidjgoss left a comment

Choose a reason for hiding this comment

davidjgoss Nov 24, 2020 • edited Loading

Choose a reason for hiding this comment

module.createRequire(filename)#

merceyz Nov 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dawn-minion commented Nov 25, 2020

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

dawn-minion commented Nov 26, 2020

davidjgoss left a comment

Choose a reason for hiding this comment

dawn-minion commented Aug 26, 2020 •

edited

Loading

davidjgoss commented Oct 5, 2020 •

edited

Loading

charlierudolph commented Oct 10, 2020 •

edited

Loading

davidjgoss Nov 24, 2020 •

edited

Loading

merceyz Nov 25, 2020 •

edited

Loading

davidjgoss Nov 24, 2020 •

edited

Loading

merceyz Nov 25, 2020 •

edited

Loading