Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Circular dependency between the cli and @cucumber/gherkin? #56

Closed
temyers opened this issue Dec 27, 2022 · 8 comments · Fixed by cucumber/gherkin#92
Closed

Circular dependency between the cli and @cucumber/gherkin? #56

temyers opened this issue Dec 27, 2022 · 8 comments · Fixed by cucumber/gherkin#92
Assignees

Comments

@temyers
Copy link

temyers commented Dec 27, 2022

I think there's a circular dependency for the CLI between this package and @cucumber/gherkin.
Should the CLI be moved to be co-located?

👓 What did you see?

cucumber/gherkin#64

During the implementation of the above, I was attempting to update ast.ndjson generated files for python and javascript versions.
Updates to the Javascript token matcher were not reflected in the acceptance tests.

✅ What did you expect to see?

📦 Which tool/library version are you using?

  • See above branch for Gherkin version
  • Running using the cucumber/cucumber-build:latest docker container for development environment (using VSCode devcontainers)

🔬 How could we reproduce it?

Steps to reproduce the behavior:

  1. Checkout the Python enable markdown acceptance tests gherkin#64
  2. cd javascript
  3. make acceptance

📚 Any additional context?

Extensive comments in the referenced pull request.


This text was originally generated from a template, then edited by hand. You can modify the template here.

@temyers temyers changed the title Circular dependency with @cucumber/gherkin? Circular dependency between the cli and @cucumber/gherkin? Dec 27, 2022
@mpkorstanje
Copy link
Contributor

Well. This is a pickle...

@mpkorstanje
Copy link
Contributor

@mattwynne the acceptance tests for Gherkin use npx gherkin-javascript which pulls in the Gherkin version from NPM rather then the one being build locally.

It looks like Gherkin Streams was extracted so it could provide a CLI for Gherkin without being bundled with Gherkin. And so I don't think this module needs to exist. @mattwynne I reckon merging this back into Gherkin makes most sense, but it shouldn't be published. Do you know how to to do that?

@mpkorstanje
Copy link
Contributor

Does look like at least the html formatter is using gherkin-streams. Might be worth to check out what the dep tree looks like.

@temyers
Copy link
Author

temyers commented Jan 19, 2023

@mpkorstanje any updates on this one?

@mpkorstanje
Copy link
Contributor

mpkorstanje commented Jan 19, 2023

No, I'm too backlogged too look into this right now. Trying to offload a bit of work to other people.

@mattwynne
Copy link
Member

@davidjgoss do you have any capacity to get your head around this?

@davidjgoss
Copy link
Contributor

davidjgoss commented Jan 19, 2023

IIRC the motivation for splitting this out wasn't so much to make a CLI separate, but to uncouple Gherkin itself from Node.js streams so that the core Gherkin functionality could be used in a browser (or Deno now, I guess) environment. @aslakhellesoy tell me if I'm making things up!

I'll take a look at this later and see if we can get rid of the circular dep without merging the packages.

Note that eventually we should be able to pivot to WHATWG streams and thus get rid of the Node.js specificness.

@aslakhellesoy
Copy link
Contributor

That’s right. Breaking the node dependency so it can run in a browser (html reports) was the motivation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants