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

TypeScript Boilerplate #17

Closed
wants to merge 22 commits into from
Closed

Conversation

EdJoPaTo
Copy link

@EdJoPaTo EdJoPaTo commented Feb 19, 2019

Add a TypeScript Boilerplate to create node modules fast.

Current problems I see

These things stop this currently from being a proper boilerplate

Open questions I have

I would like other thoughts on these one.

  • Also a cli boilerplate in TypeScript? The main benefit I see is easier and less error prone usage from dependants. The cli does not do many things so I currently don't think it will be as useful as the module itself in TypeScript.
  • Prepare as build script in package.json. npm-scripts describes prepare, prepack and prepublishOnly. I personally like to check package content via npm pack so prepublishOnly is not helpful here. prepare also runs on a npm install without arguments as its done after a git clone. This might save some time as it hasn't to be done manually but could be annoying on WIP feature branches. The safest solution would be prepack but I think prepare is a nice to have.
  • Omit index.js / index.d.ts in package.json types and main? This may not be clear to a inexperienced user. Add it or omit it? For example got is currently omitting it.

closes #16

"test": "xo && ava"
},
"main": "dist",
"types": "dist",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally prefer to put non-standard package.json properties at the end.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My idea here is that it nearly does the same as main does.

typescript/source/index.ts Outdated Show resolved Hide resolved
typescript/test/test.ts Outdated Show resolved Hide resolved
"node": ">=6"
},
"scripts": {
"build": "del dist && tsc",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should open an issue on TypeScript for tsc to get a --cleanup flag or something so we don't need del dist.

By "we", I'm hoping you can do it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As of reading through a bunch of TypeScript Issues:

They don't like to delete a folder completely (good summary microsoft/TypeScript#13722) but deleting output files when the source files are gone is an ongoing Issue (microsoft/TypeScript#16057).
Sadly this issue is not planned for one of the next releases (attached as a Milestones).

},
"scripts": {
"build": "del dist && tsc",
"prepare": "npm run build",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why prepare? I usually use prepublishOnly.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prepare as build script in package.json. npm-scripts describes prepare, prepack and prepublishOnly. I personally like to check package content via npm pack so prepublishOnly is not helpful here. prepare also runs on a npm install without arguments as its done after a git clone. This might save some time as it hasn't to be done manually but could be annoying on WIP feature branches. The safest solution would be prepack but I think prepare is a nice to have.

Copy link
Author

@EdJoPaTo EdJoPaTo Mar 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed to prepack as prepare runs into errors with a fresh start repo. As prepare is not needed anyway use prepack as it runs on npm pack and npm publish.

typescript/package.json Outdated Show resolved Hide resolved
"@typescript-eslint/promise-function-async": "off",
"ava/no-ignored-test-files": "off"
}
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A goal of mine is to get rid of a lot of the config boilerplate here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. But sadly we are not there yet. It's currently the way to go so until its possible to do it more clean I would like to keep the current state here. That way everyone can see what still has to be done.

@sindresorhus
Copy link
Owner

  • Also a cli boilerplate in TypeScript? The main benefit I see is easier and less error prone usage from dependants. The cli does not do many things so I currently don't think it will be as useful as the module itself in TypeScript.

Unless the CLI is large, like XO/AVA large, I will probably not bother to make CLI's in TS.

@sindresorhus
Copy link
Owner

  • Omit index.js / index.d.ts in package.json types and main? This may not be clear to a inexperienced user. Add it or omit it? For example got is currently omitting it.

This is moot since it's being compiled to dist, so we have to list it.

@EdJoPaTo
Copy link
Author

EdJoPaTo commented Mar 5, 2019

  • Omit index.js / index.d.ts in package.json types and main? This may not be clear to a inexperienced user. Add it or omit it? For example got is currently omitting it.

This is moot since it's being compiled to dist, so we have to list it.

I assume we talked past each other: main and types are needed. But it can be dist/index.js or dist. In the second case NPM will assume the index.js. For new users it might be not clear what is happening there so it could be nice to just write the full form. Omitting it and just write dist looks more clean on the other hand (and that's the way got is going for example).

Copy link
Contributor

@Richienb Richienb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this repository is a template, adding a folder for the typescript version might defeat the point of a "template" since an extra step is added to either expand or remove the folder. I suggest moving this code to a separate template repository.

@EdJoPaTo
Copy link
Author

@Richienb This PR was added before changing this repo from multiple templates inside folders into multiple templates in multiple repositories.

I followed the migration into a template repository like 90ddca2 did.
Since then the purpose of this repo is a different one so this PR is kinda irrelevant now.

For my typescript template head over to EdJoPaTo/typescript-node-module-boilerplate, PRs and suggestion for improvement welcome :)

@EdJoPaTo EdJoPaTo closed this Jan 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeScript Boilerplate
3 participants