aurelia / skeleton-navigation

Starter kits for building a standard navigation-style app with Aurelia.
Creative Commons Zero v1.0 Universal
732 stars 515 forks source link

skeleton-typescript* restructure: move custom_typings folder to an external repo #419

Closed atsu85 closed 8 years ago

atsu85 commented 8 years ago

Hi, I propose moving custom_typings folders from all TypeScript skeleton projects to typings subfolder named custom. The typings folder contains other TypeScript declaration files managed by typings tool, so it makes sense to use it for all TypeScript declarations, including those that are not managed by typings tool.

Based on the reply from @axwalker I think this proposal already has some supporters. @Vheissu, what do You (as the original author of custom_typings folder) think about this proposal?

The requested changes are minimal (move the folder and reflect the change in tsconfig.json) and I could send the PR if this idea finds few more supporters. I know there is another issue related to removing custom_typings folder instead, but I voted against it.

EisenbergEffect commented 8 years ago

It makes sense to me.

Vheissu commented 8 years ago

Sounds good @atsu85 I think it would be nicer inside of the typings directory.

atsu85 commented 8 years ago

OK, I'll send a PR for skeleton-typescript and skeleton-typescript-webpack in few days (can't make it today) and I'll make sure both of them work as expected, including tests (that currently fail with skeleton-typescript-webpack - i'll create another issue for that later and send another PR for that).

I could need some help with skeleton-typescript-asp.net5 - at least someone to validate, that everything works as exected after the changes that i might do "blindly".

axwalker commented 8 years ago

I think there's a much better solution here. I don't think the typings folder should be be in the repository at all - and I think it's correctly listed in .gitignore. tsconfig.json and typings.json are all that you should need. If you:

  1. Delete the typings folder.
  2. Remove "./node_modules/aurelia-*/dist/aurelia-*.d.ts", from the filesGlob in tsconfig.json. Otherwise you get duplicate declaration errors.
  3. Add "es6-collections": "registry:dt/es6-collections#0.5.1+20160316155526", to typings.json.

Once all this is in place, typings install will do all the work for you.

I think the only file you need in custom-typings is aurelia-protractor.d.ts - which is all that is left in the typescript-webpack skeleton. With that in mind, I think it'd be best for that to be hosted somewhere and then included in typings.json along with the rest of the aurelia typings.

Then we don't need to have custom-typings in the repository at all and it's up an individual user to organise their own custom typings when they need them for their own personal dependencies.

atsu85 commented 8 years ago

@axwalker, I agree with most what You said:

I don't think the typings folder should be be in the repository at all

I also think that for real project there is no need to add derived resources to the version control, but this was decided the other way in much earlier discussion that I didn't participate. To be honest, I don't really care if files created by typings install are in the git or not.

Remove "./nodemodules/aurelia-/dist/aurelia-_.d.ts", from the filesGlob in tsconfig.json. Otherwise you get duplicate declaration errors.

I also found out, that it isn't needed any more, because the same files are now created by typings install

Add "es6-collections": "registry:dt/es6-collections#0.5.1+20160316155526", to typings.json.

Yes, I guess the best suggestion is to run typings install es6-collections -SA to create that line to typings.json. And in case of skeleton-typescript-webpack it is already managed by typings.

I think the only file you need in custom-typings is aurelia-protractor.d.ts - which is all that is left in the typescript-webpack skeleton

Correct, in case of skeleton-typescript-webpack it is the only file in custom_typings, but skeleton-navigation-typescript-vs/custom_typings also contains es6.d.ts and skeleton-typescript/custom_typings also contains core-js as well.

With that in mind, I think it'd be best for that to be hosted somewhere and then included in typings.json along with the rest of the aurelia typings.

Basically I agree with You, but when the user wants to experiment with some lib, that doesn't have official TypeScript declarations, then he/she already has a place where to start writing the custom declarations. Currently I use this typings/custom folder a lot. Maybe I should be much better OS dev and share them with the rest of the community, but I've been a bit lazy (and just wanted to get my stuff done as fast as possible for my day-job). So It might still make sense to at least keep the "empty" folder and let the build task include it when compiling TypeScript sources. To be honest, for I don't really care about this folder at all, I'm familiar with TypeScript echosystem, but it is the newcomers that I'm most concerned of.

Anyway, I guess this issue still needs some discussion, so I won't rush with creating the PR.

RomkeVdMeulen commented 8 years ago

I still suggest moving the typings currently in custom_typings to an external repo so that people can pull in updates and fixes with the typings command: that's the whole point of that tool.

I don't mind keeping the custom_typings folder (if you must) so that new developers have a place to put their own type files that will automatically be picked up. But at least make it empty: it's a bad idea to have people copy the type files into hundreds of projects and never updating them.

Worst case scenario is if it turns out there's an serious error in custom_typings/es6.d.ts. The way things are now, every developer who has ever based their project on the typescript skeleton will have to manually copy-paste the correction into their project. If the typings are in an external repo, the fix can be applied there and every developer need only run typings install to pull the changes in. Much better, in my opinion.

RomkeVdMeulen commented 8 years ago

To illustrate: one of the first things I did once I started my current project, based on the typescript skeleton, was to get rid of custom_typings completely and use typings.json instead. github:DefinitelyTyped/DefinitelyTyped/es6-shim/es6-shim.d.ts makes a handy replacement of es6.d.ts, though there are numerous other options. The core-js.d.ts isn't necesary anymore since Aurelia replaced it with aurelia/polyfills. I'm not sure what aurelia-protractor.d.ts is used for - I got rid of it without problems, but if it's necessary it can be externalized easily (like I did here).

axwalker commented 8 years ago

aurelia-protractor.d.ts is for the e2e tests in the TypeScript skeletons.

niieani commented 8 years ago

I agree with @RomkeVdMeulen that some of the typings we ship with the skeletons should be separated out to their own repo. Perhaps we could wait until the new, NPM-way of installing typings is released.

EisenbergEffect commented 8 years ago

@niieani I was thinking of setting up a new repo called dts that we could drop any external or custom dts files in, then ad a typings.json file there so eveything in that repo can be easily installed. What do you think? We can put the protractor, fetch, i18next and validatejs typings in there. Let me know and I'll get it done right away.

Vheissu commented 8 years ago

I am definitely onboard for the idea of a dts repository. Will make the skeletons even cleaner and easier to maintain the typings. On 25 Jun 2016 1:00 PM, "Rob Eisenberg" notifications@github.com wrote:

@niieani https://github.com/niieani I was thinking of setting up a new repo called dts that we could drop any external or custom dts files in, then ad a typings.json file there so eveything in that repo can be easily installed. What do you think? We can put the protractor, fetch, i18next and validatejs typings in there. Let me know and I'll get it done right away.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/aurelia/skeleton-navigation/issues/419#issuecomment-228504913, or mute the thread https://github.com/notifications/unsubscribe/AAWeyifbKsBxX2nliNGtyJss0u1cv5Y1ks5qPJnVgaJpZM4IRMx2 .

niieani commented 8 years ago

Sounds good, let's do that! Note that we can use the fetch from DefinitelyTyped, we only need an extra URLSearchParams interface, see here: https://github.com/aurelia/skeleton-navigation/blob/master/skeleton-typescript-webpack/custom_typings/fetch.d.ts. That interface seems to be Aurelia-specific (non-standard fetch). Perhaps that interface should be declared directly in aurelia-fetch-client?

niieani commented 8 years ago

However, I would leave the custom_typings folder be, even if it's empty. It's a good practice to have a separate folder for custom typings. We can't move it to typings/custom either, because typings is in .gitignore which would mean those files would not be committed.

EisenbergEffect commented 8 years ago

Good advice. I'll take care of this either this weekend or first thing on Monday. I didn't realize the URLSearchParams was part of our API. I can easily add an interface to that library. I'll check it out. If we can use that plus the definitely typed fetch that would be a big help to the community.

niieani commented 8 years ago

As to the package/repo name aurelia-typings might be clearer as to its purpose than aurelia-dts.

EisenbergEffect commented 8 years ago

Sounds good.

EisenbergEffect commented 8 years ago

@niieani I don't see fetch on definitely typed. I see a isomorphic fetch which defines its own module. Is that what you are referring to? If so, is that safe to use?

niieani commented 8 years ago

isomorphic-fetch typing is a bit outdated, the best one is whatwg-fetch (which is the module used by isomorphic-fetch in the browser). https://github.com/aurelia/skeleton-navigation/blob/master/skeleton-typescript-webpack/typings.json

When using whatwg-fetch we could add a file which aliases that package to either fetch or isomorphic-fetch, depending on the one used. isomorphic-fetch is better in that it also works under node (so makes more sense with the upcoming support for server-side Aurelia.

Aliasing is as simple as:

// 'fetch' is global when using the typings whatwg-fetch
declare module "isomorphic-fetch" {
  export = fetch;
}

See: https://github.com/aurelia/skeleton-navigation/blob/master/skeleton-typescript-webpack/custom_typings/fetch.d.ts

EisenbergEffect commented 8 years ago

@niieani We've had the whatwg-fetch dts in our fetch repo for some time, but I'm not sure where that came from. Is there an official source for that? Is there some way we can install from that official source into the skeletons? or do we just need to copy that into our typings repo for now?

EisenbergEffect commented 8 years ago

Here's what I've put up: https://github.com/aurelia/typings But I wasn't sure how to write the typings.json itself, since there's no main.

niieani commented 8 years ago

@EisenbergEffect looks good. DefinitelyTyped is the official source, so we could use that + alias isomorphic-fetch instead of maintaining a copy of whatwg-fetch there. I think typings.json doesn't need a main, but a list of dependencies: globalDependencies (like the whatwg-fetch one from DefinitelyTyped and list of our files from dist. Not sure how exactly it works though, I'd imagine when you typings install github:aurelia/typings it would then install of its dependencies automatically.