-
Notifications
You must be signed in to change notification settings - Fork 22
feat(typescript): use Typescript index + many changes #407
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
Conversation
|
.d.ts can en added to projects that don’t use typescript themselves. I think “got” is an example for a package like that |
Got is made in typescript (at least now), I will try to find npm package with this pattern, but I'm not sure it's worth slowing down the script that much for the real potential it brings. |
|
The most popular packages have a .d.ts and aren't written in TS:
That was just the first few results, so we can't skip those IMO :) |
I'm not saying your point is bad, but like I said this cover almost every package. I will add a check on the |
|
@Haroenv I have removed the check for dependencies as it was not as certain as I thought.
I would recommend checking only b4b98ef (without whitespace diff) |
Haroenv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're almost ready here
| pkgs.map((pkg, index) => { | ||
| const fromFS = checkChangelogFromFilesList(pkg, filesLists[index]); | ||
| if (fromFS) { | ||
| return fromFS; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hope this will have a nice impact! probably not too much since GitHub is fast, but less request is better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has less impact than I initially anticipated because some packages does not bundle the changelog (most of them do though, because npm auto add this file), and we always rollback to slower method if not found.
src/typescript/index.js
Outdated
| // t = @types/<name> | ||
| body.forEach(type => { | ||
| type.m.forEach(m => { | ||
| typesCache[m] = type.t; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how does this deal with @reach/router -> @types/reach__router?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will work (if you look here it's already correctly name)
Your example made me found out that not all packages has a property m in the json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example mapbox__polyline does not contain the correct package name, reach__router seems to have the same issue. I was expecting to have the correct package name in m but it seems not consistant.
(maybe @sandersn has an idea)
If no solution I will unescape the name probably to transform the __ into a /
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure that the module name is supposed to be the DT-mangled name, eg mapbox__polyline instead of @mapbox/polyline. Do you see unmangled package names too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example: @mapbox/geojson-area or arangodb appear as-is in the m property.
I'm really not sure what should or should not be mangled
|
I added an unmangle function to fix this #407 (comment) |
| } | ||
|
|
||
| function unmangle(name) { | ||
| return name.replace('__', '/').replace('@', ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add a test for this? Shouldn't we add an @ instead of removing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can not add an @ because the index created by typescript also contain "fake module" like lodash/add (I'm not 100% sure I need to add those in the list)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that's confusing
|
I didn't check whether it's handled, but don't forget to handle the case where a package has a Context: sindresorhus/type-fest#67 (comment) |
|
good call @sindresorhus, I think that's not checked for now, if the main field isn't given. With this refactor that would indeed not be fixed |
|
Hey I was wondering on the status of this. Maybe it's good to get merged? It has some interesting features |
|
@jimaek hey, thanks for pinging. I'll try to allocate some times to this in the coming weeks |
|
Everything has been ported 🙇🏻 |
Closes #401
Contributes #372
📦 What's inside ?
@types, preloaded like jsDelivrWe should probably just restart the script on heroku once a day, until we code something to handle the cache automatically.
dependenciesanddevDependenciesto see if package usetypescriptIt's a super light check and I'm almost sure all packages that are using typescript must have them listed as deps.
Example: in the snapshot we solved
atomic-package.tab.d.tsthat was listed as possibleThat also mean we no longer use
unpkgfor the moment, that will help going faster.