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

Feedback on generated client package.json #1618

Closed
workeitel opened this issue Oct 25, 2020 · 3 comments
Closed

Feedback on generated client package.json #1618

workeitel opened this issue Oct 25, 2020 · 3 comments
Assignees
Labels
closed-for-staleness feature-request New feature or enhancement. May require GitHub community feedback. investigating Issue is being investigated and/or work is in progress to resolve the issue.

Comments

@workeitel
Copy link
Contributor

workeitel commented Oct 25, 2020

Is your feature request related to a problem? Please describe.
I try to generate non-public clients outside the repository. I noticed some issues with the generated clients. The current generated scripts in package.json:

  "scripts": {
    "clean": "npm run remove-definitions && npm run remove-dist",
    "build-documentation": "npm run clean && typedoc ./",
    "prepublishOnly": "yarn build",
    "pretest": "yarn build:cjs",
    "remove-definitions": "rimraf ./types",
    "remove-dist": "rimraf ./dist",
    "remove-documentation": "rimraf ./docs",
    "test": "yarn build && jest --coverage --passWithNoTests",
    "build:cjs": "tsc -p tsconfig.json",
    "build:es": "tsc -p tsconfig.es.json",
    "build": "yarn build:cjs && yarn build:es"
  },
  1. Some commands using npm, some yarn. It looks like no yarn specific feature is used so I think npm works for everybody while yarn introduces another dependency. it would be great if it's a) consistent and b) it works without yarn or at least support both (like https://stackoverflow.com/questions/41647961/package-json-scripts-that-work-with-npm-and-yarn )
  2. clean does not clean the documentation. I would expect after clean everything is gone
  3. build-documentation cleans everything (except docs) which looks unnecessary
  4. build-documentation should be named with colon: build:documentation
  5. prepublishOnly should run tests and generate documentation to ensure everything works before publishing
  6. pretest builds cjs but test executes a full build anyway so pretest is not necessary
  7. for faster iteration it would be great if tests are executed with ts-jest preset to test the TS directly without build.
  8. No tests included in the generated client. Is jest even needed?
  9. the generated jest.config.js includes ../../jest.config.base.js which is not generated. It would be better if the client is self-contained. The shared jest config only contains one value which looks pretty standard. Should that be skipped to avoid a dependency to the mono-repo?
  10. The different remove-* targets look inconsistent with build: tasks. Maybe change to remove:dist with colon?
  11. In addition files: ["dist", "types"] would be cleaner than the current .npmignore as mentioned in fix(codgen): only publish types and dist folder to npm #1615

Edit 2021-05-22: Item 2., 3., 5., and 8. are already solved in the meanwhile.

Describe the solution you'd like

I suggest the following instead:

  "scripts": {
    "build": "npm run build:cjs && npm run build:es && npm run build:documentation",
    "build:cjs": "tsc -p tsconfig.json",
    "build:documentation": "typedoc ./",
    "build:es": "tsc -p tsconfig.es.json",

    "clean": "npm run clean:definitions && npm run clean:dist && npm run clean:documentation",
    "clean:definitions": "rimraf ./types",
    "clean:dist": "rimraf ./dist",
    "clean:documentation": "rimraf ./docs",

    "prepublishOnly": "npm run build"
  }

I'm happy to create a pull-request with the change but I couldn't find where the file comes from.

@workeitel workeitel added the feature-request New feature or enhancement. May require GitHub community feedback. label Oct 25, 2020
@AllanZhengYP AllanZhengYP added the investigating Issue is being investigated and/or work is in progress to resolve the issue. label Nov 23, 2020
@trivikr
Copy link
Member

trivikr commented Mar 18, 2021

Action required: Do not inherit from global jest config in clients folders.

@github-actions
Copy link

Greetings! We’re closing this issue because it has been open a long time and hasn’t been updated in a while and may not be getting the attention it deserves. We encourage you to check if this is still an issue in the latest release and if you find that this is still a problem, please feel free to comment or open a new issue.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels May 29, 2022
@github-actions github-actions bot closed this as completed Jun 3, 2022
@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
closed-for-staleness feature-request New feature or enhancement. May require GitHub community feedback. investigating Issue is being investigated and/or work is in progress to resolve the issue.
Projects
None yet
Development

No branches or pull requests

3 participants