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

Is there another way to distribute typings.d.ts? #518

Closed
jorgebucaran opened this issue Dec 30, 2017 · 39 comments
Closed

Is there another way to distribute typings.d.ts? #518

jorgebucaran opened this issue Dec 30, 2017 · 39 comments
Labels
discussion meta News and communication question How do I do that?

Comments

@jorgebucaran
Copy link
Owner

jorgebucaran commented Dec 30, 2017

TypeScript as a dev dependency adds 33MB to node_modules. I want to support TypeScript, but I don't want to add this weight to every Hyperapp install. Is there any way we can still distribute typings.d.ts without having TypeScript as a dev dependency on this repo?

What other options do we have?

@jorgebucaran jorgebucaran added meta News and communication discussion question How do I do that? labels Dec 30, 2017
@timjacobi
Copy link

timjacobi commented Dec 30, 2017

We could publish the typings to DefinitelyTyped and tell users that want TypeScript support to install them via npm install @typings/hyperapp.

On the other hand if we are in the TypeScript zone already why don't we write the library itself in TypeScript. Andre Staltz argues that "All JS Libraries should be authored in TypeScript
"
(or another typed JS layer) and I kinda agree with him.

@okwolf
Copy link
Contributor

okwolf commented Dec 30, 2017

I'm a bit confused - I thought that TypeScript support comes from adding "typings": "hyperapp.d.ts" to our package.json and including that file when we publish.

I also thought the only reason we install TypeScript was to run the test that verifies our typings match the current library code. We could move this verification to another project since hopefully the API changes should be minimal in a post 1.0.0 world 🙏

@frenzzy
Copy link
Contributor

frenzzy commented Dec 30, 2017

The best place for typings are DefinitelyTyped and flow-typed if source code is plain javascript because they have tests for typings, compatibility checks with typed language version, etc.
I vote for removing typings from core :)

@okwolf
Copy link
Contributor

okwolf commented Dec 31, 2017

So if we do decide to publish on DefinitelyTyped instead, that might raise the question again of where our API docs are. I think we're going to need to write some if we want to be taken seriously and increase adoption. With the stable 1.0.0 API, this should be worth the effort.

@jorgebucaran
Copy link
Owner Author

@okwolf Where are they now?

@okwolf
Copy link
Contributor

okwolf commented Dec 31, 2017

@jorgebucaran I thought our current answer is that hyperapp.d.ts is our only API documentation.

@jorgebucaran
Copy link
Owner Author

@okwolf Gotcha. Yeah, you are right.

@Seikho
Copy link

Seikho commented Dec 31, 2017

Is having TypeScript as a dev dependency a big deal? The only people this matters to are the people who clone the project and install its dependencies.

Leaving the type definitions in this repository will lower the overhead of the process of updating the dependencies. It means cloning DefinitelyTyped (which is well over 33MB) and submitting a pull request there.
By leaving the type definitions here, you'll be able to update and release them quite a lot faster.

It also means that the type definitions included in a typical install of hyperapp should match the version that they've installed.

@jorgebucaran
Copy link
Owner Author

@Seikho We would lose some of that if we removed the typings from this repo. As a compromise, we could ship the typings, but not require TypeScript as a dependency like @okwolf said.

@Swizz
Copy link
Contributor

Swizz commented Dec 31, 2017

And how about maintaining our own typing repo ?
People will install @hyperapp/typed and we will be able to provide types for Typescript, Flow, Reason, whatever without noise in the main repository

@jorgebucaran
Copy link
Owner Author

That's another good idea.

@alber70g
Copy link
Contributor

alber70g commented Dec 31, 2017

Please don't remove the typings from the repo. Just leave it there and remove the typescript dependency as people who use typescript have it probably already installed anyway.

And sorry, but 33MB... it's next to nothing compared to when you do an npm install in a normal project ;)

@Swizz
Copy link
Contributor

Swizz commented Dec 31, 2017

@alber70g I am without. I hate using DefinitelyTyped and it is really a please to only have to depend on the lib you want to use (Hyperapp) instead of having to depend on one more package to have the types.

Do not forget we will have to do the same with the router and html.

  "dependencies": {
    "@hyperapp/HTML": "^0.5.1",
    "@hyperapp/router": "^0.4.1",
+   "@types/hyperapp": "^1.0.0",
+   "@types/hyperapp-html": "^0.5.1",
+   "@types/hyperapp-router": "^0.4.1",
    "hyperapp": "^1.0.0"
  }

@Swizz Swizz closed this as completed Dec 31, 2017
@Swizz Swizz reopened this Dec 31, 2017
@timjacobi
Copy link

We could also make TypeScript and optional dependency. If you don't want to install it you run npm install --no-optional

@SteveALee
Copy link

I'm late to the party and don't have much new to add to the thread, but having used TS for a while here are my 2cs

  • Writing a library in TS means the typings are guaranteed to be be correct. As mention the typings are the AP docs. This is ideal
  • It's easy to build and publish the library npm package so that both the pure JS and typings are included. Thus it's usable by JS or TS users without any extra effort. This is ideal and now the preferred way according to TS community.
  • Yes the typings will be included in using codes deps (node_modules) but it's dev only and they don't end up in the built app bundle.
  • definitelyTyped is still available but depreciated. Ambient types for JS libs are now published under npm @types/*. VSCode is really good at finding the correct typings wherever they are but other tools need more help.
  • TSC replaces babel for transcompiling to ES5, though you still need polyfils for promises etc. TSC is pretty flexible in what ES version you target.
  • You can gradually migrate the code to TS , but then the typings will not be complete unless you maintain them separately. Also you can now add type docs in JS files for the compiler to see, but I doubt typings are created in this case (i've not checked)

My preference would be be to get to the point where the library is written in TS and published with the typings in npm package for anyone using TS. However I accept this is extra work for the library maintainers and the types can get pretty complex, especially when generics are used (see cyclejs)

@jorgebucaran
Copy link
Owner Author

@timjacobi Not too bad :)

@SteveALee Please see #494

@SteveALee
Copy link

@jorgebucaran yeah, the 2 are tightly coupled :)

@SteveALee
Copy link

SteveALee commented Dec 31, 2017

@jorgebucaran Hmm, I got a bit carried away and answered more than this issue. Sorry

Given the typings are to be maintained separately from the javascript (#494) they are so called ambient types like any others in definitetyped etc. As a TS user of the hyperapp lib I'd like them to be easily available and while that means having them in the main npm package I'd still accept them being available via adding @types/hyperapp as another dependency, especially as I believe @code will still find them anyway (if names are consistent).

@jorgebucaran
Copy link
Owner Author

@timjacobi's or @okwolf suggestions are pretty good :)

@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Jan 3, 2018

Thank you, everyone, for your feedback. I think I'll keep things the way they are for now, or use @okwolf's idea, either way, we'll keep the typings on this repo.

@only-cliches
Copy link

only-cliches commented Jan 7, 2018

I saw this a few days ago, thought I'd add my two cents even though it's been closed.

Since you're not distributing the library in Typescript form, you could easily remove Typescript from devDependencies all together since it's technically not a dependency.

Really, the only part of the lib that requires Typescript is the tests. That can be resolved by replacing the test command with (npm list typescript || npm i typescript) && jest --coverage --no-cache && tsc -p test/ts to make sure it's installed if tests are being ran.

@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Jan 7, 2018

Hey @ClickSimply! Wow, that's really ingenious:

(npm list typescript || npm i typescript) && jest --coverage --no-cache && tsc -p test/ts

Sneaky! 🐍

@okwolf
Copy link
Contributor

okwolf commented Jan 7, 2018

@ClickSimply or you could just simply do jest --coverage --no-cache && npx typescript -p test/ts

@jorgebucaran
Copy link
Owner Author

@okwolf But then we need to install npx! 😏

@okwolf
Copy link
Contributor

okwolf commented Jan 7, 2018

@jorgebucaran if only npx came with node 8+. Oh wait...

@jorgebucaran
Copy link
Owner Author

@okwolf It does?

@okwolf
Copy link
Contributor

okwolf commented Jan 7, 2018

@jorgebucaran technically npx comes bundled with npm >= 5.2.0, which comes bundled with Node.js >= 8.2.0 🙄

@only-cliches
Copy link

My suggestion was intentional, if you use npx it will take the time to install typescript every time you run the test. The provided command will only install it the first time if it isn't already there.

Either case should work out fine and allow Typescript to be removed from the package file.

@jorgebucaran
Copy link
Owner Author

Thanks, for clarifying @ClickSimply, I thought npx would do that too, but as it turns out it doesn't. I wonder why.

@only-cliches
Copy link

It's a feature; part of the point of npx is to keep your environment clean so it removes all the dependencies required to run the command after it's finished.

@Swizz
Copy link
Contributor

Swizz commented Jan 7, 2018

Now we reach 1.0.0 the API is not going to change a lot. Do we really need to test typing ?
Answering no could totally solve the issue.

@jorgebucaran
Copy link
Owner Author

How common is it to test types? Just curious.

And btw, the types are probably going to change again soon (/cc @alber70g), so we may want to put this on hold until then.

@SteveALee
Copy link

Well that’s a good point. The types only change if the api changes. But, if they are published then they need to be tested first.

Given they are effectively an extra part that is published with the main code why not add the tsc install and types test to a separate prepublishOnly, script? That won’t then run in automated tests like CI so I’d actually put running of types test in its own script that can be run separately and call that from prepublishOnly. The reason not to use prepublish is that get calls from install too.

That way, the types get tested before publishing but the over head of installing typescript (wors if using npx) is not part of running the main tests.

@okwolf
Copy link
Contributor

okwolf commented Jan 7, 2018

npx only does an on-demand install if it's not already available. It gives the user control by first checking if the package is installed in node_modules or globally first.

@only-cliches
Copy link

only-cliches commented Jan 7, 2018

Maybe I'm using it wrong then? It installs Typescript and runs it from the temporary location every time I run it. Might be a Windows thing.

typescript

Edit: Hmm, seems like there's an open issue with npx regarding this. Might just be a temporary bug.

Documented behavior is exactly as you described okwolf. I've just never seen it. ;)

@styfle
Copy link

styfle commented Jan 12, 2018

@jorgebucaran

TypeScript as a dev dependency adds 33MB to node_modules. I want to support TypeScript, but I don't want to add this weight to every Hyperapp install.

Who are you worried about downloading the 33MB?
The way I see it, there are only two groups of people who could be affected: contributors or users.

  • The contributors run git clone https://github.com/hyperapp/hyperapp && npm install
  • The users run npm install --save hyperapp

Because the users will not get typescript because its a devDependency. Only the contributors will get it which makes sense because they need to run tests. So any way you slice it, at some point a contributor will need to install typescript via npm install or npx or copy files from a tar ball....it all ends up on their computer to run the tests.

@SteveALee
Copy link

I've gotta say my thoughts and preference are in line with @jorgebucaran. The offered solutions do work but add complexity to save devs a few meg storage.

@only-cliches
Copy link

@styfle

Because the users will not get typescript because its a devDependency.

This isn't correct. npm will install devDependencies unless the --production flag is passed to the npm install command OR if the NODE_ENV is set to production.

Source: https://docs.npmjs.com/cli/install

@styfle
Copy link

styfle commented Jan 13, 2018

@ClickSimply I think you missed a key point in my post above....

When I say "users" I mean the users of hyperapp...the people who install hyperapp. These people will NOT get TypeScript, and I can prove it.

Execute the following:

mkdir my-app
cd my-app
npm init -y
npm install --save hyperapp
# + hyperapp@1.0.1
# added 1 package in 1.338s
ls -l node_modules
# total 4
# drwxr-xr-x 1 styfle 197609 0 Jan 13 17:09 hyperapp/
du -sh .
# 82K     .

And there you have it, the only dependency install is hyperapp and its only 82K, not 33M.

This is because hyperapp has no dependencies, only devDependencies.

However, the "contributors", the people who develop hyperapp and contribute Pull Requests will get the devDependencies when they clone hyperapp and run npm install. Which is basically what your post describes and your link to npm cli.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion meta News and communication question How do I do that?
Projects
None yet
Development

No branches or pull requests

10 participants