Remove browser.d.ts
#151
Comments
How about adding Maybe server side code uses |
I would argue to allow more types (webworker) but I'm okay with not making it a default. For my use case, it's Universal JavaScript (in particular case Angular 2 where we run Angular 2 on the server/client/webworker). I'm totally fine with a flag or, at least, options in |
I just tried Typings because tsd says it's deprecated. And it is not a drop in replacement. I'm getting ton of duplicate errors now. How do I resolve that for good? So that if I do a clean checkout I can easily just do typings install and get going? How is this even intented to be used? Unless I have each *.ts file specified in tsconfig individually this will break for everyone almost immediately. How is this a sane default? This issue needs to be escalated. |
@oliverjanik |
@unional thanks! I've updated the original question to point out that one needs to exlcude just Nevertheless great work everyone. Thanks for trying to make the world a better place ❤️ 🌹 |
@basarat you are welcome. You are making the world a better place just by giving out bunch of 🌹 🌹 😄 |
Works, cheers. Maybe put this into intro docs? |
@oliverjanik Like https://github.com/typings/typings#maindts-and-browserdts? @basarat Browser is the tree generated by following browser resolution (see https://github.com/defunctzombie/package-browser-field-spec). It's not possible to create this easily yourself, hence Typings is currently generating it. 99% of the time, these files are likely the same and I agree it's confusing. The issue is that 1% extra that may need the browser typings that are a tiny bit different to the node version, and TypeScript does not support this using native module resolution. Anyway, I can understand the friction issue, it's just there's not a good solution. For most users, it's a quick exclude or add one entry to files. For the reverse support, however, it's incredibly painful. To see how one might use this, I've open sourced this little app I wrote for a presentation last week: https://github.com/blakeembrey/hello-world-app. See https://github.com/blakeembrey/hello-world-app/blob/master/app/tsconfig.json#L13 (front-end) and https://github.com/blakeembrey/hello-world-app/blob/master/src/tsconfig.json#L13 (back-end). |
No, like this: "exclude": [
"node_modules",
"typings/main.d.ts",
"typings/main"
] |
There's a note that says to do that? I don't think I can write every combination of exclude that would be used. Did you see:
|
I know, but I think it's not prominent enough. I believe this should be part of quickstart. |
Sure, me too. If someone wants to contribute quick start docs, that would be great. |
I sent a PR. You can read it here https://github.com/typings/typings/tree/memo/browser#maindts-and-browserdts 🌹 |
Personally, I like the idea of an 'env' or 'envs' setting in typings.json where one could choose the "kind" of typings to include in a project. For example:
The behavior could be as follows:
I would argue that browser should be the default as there will probably be (at least for a while) proportionally more developers using TS to create Web applications (due to the influence of Angular 2). |
@basarat thank you for your example. 👍 |
Just switched from |
For everyone the issue is closed as https://github.com/typings/typings/tree/master#maindts-and-browserdts should be clear. As blake said having the user create Feel free to PR more docs 🌹 |
I do not think this issue should be closed. EDIT: See #151 (comment) While instructions were provided on how to manually select between Creating a Typescript project should not yield tons of duplicate identifier errors (I created a new project using TS 1.7.5, added some typings, a basic Hello World file, and ran @blakeembrey, you said it yourself:
So why clutter 99% of people's repos with duplicate typings? I propose any or all of following:
The goal is to add typings using only the project's commandline and have it work out of the box. And then if people need browser typings, they can add them too. Criticism, suggestions, and improvements welcome. |
You would also need to exclude things like
Because there's not a cleanly proposed alternative.
Anyway, I'm sure I brought these points up already, but I'm happy to solve it properly once we have a good solution for solving it. |
Also, who is the most common user in this use-case? Do browserify + webpack + etc. users want accurate typings from the get go without tweaks? It may be possible to remove duplicate identifiers when the content is the same, hence removing the issues for the majority until one type definition is actually different - would that help or just push the issue down the road? |
Ok, this probably won't happen for a long while, but perhaps a map with enabled additional resolutions? {
"resolution": {
"browser": true
}
} Maybe some others appear later. In this case, we'll always generate main but also add |
I didn't have to when creating a test project actually. Historically I have had to ignore
Fair enough.
I see. Just for my own understanding, in order to use both, you'd need to compile each subproject separately and include I did not fully understand the problem before, since I've only worked with projects that had a single Disclaimer: I think with separate Referring to the opening line of https://github.com/Microsoft/TypeScript/wiki/tsconfig.json
To me, this means the But now you have two projects that rely on the same typings, but different versions of them (browser and normal). Your solution was to create Basically, in solving your project structure problem, you have created a new one for everyone else. In light of the above, I think the solution to this problem will involve:
Again, I'm really sorry to rip into this. This is the strongest I've felt about something codewise. I'm going to strikethrough my previous suggestions. Discussion encouraged, because to me this is a massive breaking change. |
This is certainly reasonable, I don't take any of it personally (yet). Anyway, it depends on what angle you look at it, but you're somewhat right. I'll see what I can do for next release to change the structure. The way I saw Typings is that you're typings JavaScript projects, so it should mirror
I don't think it's a project structure problem, the project is structure correctly. Maybe you could separate For 1, we can't do directly that but we can move For 2, agreed. I suggest For 3, not sure - maybe just a flag during init. Again, I guess I treat Anyway, some things to keep into account here too. In the future, TypeScript can have multiple All in all, I'll see what I can do for 0.7 but please continue the discussion. |
Your logic makes sense, and the polyfill solution works. TS obviously can operate using a solution like the one you've made, but for the sake of being a strong partner to TS, I think Typings should aim to be installed alongside a TS root directory (which may or may not be the same as a project root directory).
That's a good point. I am mainly trying to avoid duplicate definitions.
Sounds perfect to me.
We can still accommodate this by allowing separate Another solution could be to suggest better duplicate typings handling to the TS team, but I'm unsure how they'd accomplish that, and obviously the Typings project should focus on the present and near future features.
I have no opinion on this, but it sounds like a nice idea. |
Only that it doesn't logically map with the rest of the feature. Logically, it's Edit: To be clear, it's to minimise explicit documentation and surprises. The less surprising the behaviour, the better. If |
If the 'both' option is removed, then typings wont behave as it does today, which I believe was extremely important to you. I mean, the PR can be left as is, and we just wont tell anyone there's a 'both' option, but you can still use it. |
@phestermcs I'm good with killing "both", we just need to make sure that both outputs are still possible (independently is fine, until we extend support for the object extension proposal). The reason is, the correct implementation without |
Well, I'm a little confused. Arent there projects right now that depend on both browser and main being created, like some of yours? Of the 'both' option is removed, then what happens to you and others who are currently relying on both being output? I honestly dont see what's logically or conceptually wrong with having a 'both' option, even when a later enhancement is made. I really really believed you when you said there are cases that require this today. I think you should allow 'both'. But to confirm..
yes? |
Relying on it a tiny bit, but as we decided - it's an edge-case. Either way, those projects can stay on 0.8 a couple of extra weeks (this is the reason why it's fine, doing the object enhancement on top of a solid base should be easy).
I want to kill the current structure, because it doesn't scale when we start configuring paths/output resolutions. It'd be good to use the structure I mentioned above. Edit: Link to structure: #151 (comment). |
I accept it would be good and nice to improve the 'default' structure, but can't that be part of your more 'complete' implementation? Here's thing to please consider. The PR, right now, WORKS. Can't you just 'pretend' there was an explicit object specification with Here's the thing, the more change, the more chance to break something, and fiddling with the directory 'default' structure isn't going to make the PR work any better or worse. |
@phestermcs The tests are failing on the PR, and you I'm guessing you haven't tested the uninstall feature as I mentioned, because there should be a warning if the file is not there when you try to delete (emitted https://github.com/typings/core/blob/master/src/utils/fs.ts#L71). |
So. In Travis the tests failed on Node 0.10 and 0.12, but all tests passed for for later Node version. I did test all the commands, including uninstall. But you're saying that if the typing isn't there on uninstall the user should be told that? To confirm changes to PR:
yes? |
So, uninstall is actually working correctly, in that it notifies if typings not there. However, the test is wrong now, as it's assuming two messages, rather than just one. There is no uninstall.spec.ts... What actually is failing? |
If you don't want to do the full PR, feel free to say so. I know it's a bit of work when you're unfamiliar with a project, and it touches a decent amount of the project. I can try to make it happen this weekend or next week.
Won't it notify that it can't find the browser version when you never installed the browser version with this addition?
Look at |
Ok, I dont recall ever saying I didn't want to do a 'full PR'. So what are you referring to? Are you saying you require new functionality to change the directory structure to be added in order for you to accept a minor change to resolve the pain? I'm feeling most of my 'work' is getting you to compromise as to what is a must-have vs. a nice-to-have.
There is only one failing test, it only happens on node 0.12. A failure is occurring on node 0.10 but there's some problem in that Travis goes into an infinite scroll when trying to view the test results for 0.10. The failure is not related to uninstall command, rather:
No, functionally everythings working, it's a single test in node 0.12 that is not working. Further, because you want the 'both' option removed, I can no longer have the existing tests pass, and the all must be changed. Here's what I'd like:
So, how much money do you need to bend a little here? |
I don't follow, sorry. Wasn't the initial making |
I only said it because you keep changing my statement about the directory structure. I said both times you asked it should be changed to implement the feature correctly. |
Yes, changing the default to only output 'main' rather than 'both' would be a breaking change. But of your own admission:
So yes, it's a breaking change for you. But its a FIXING CHANGE for everyone else.
I see, so even today, even though people are using typings with the current directory structure, even though my PR leaves that intact, even though that works just fine, even though a future enhancement allowing an explicit structure the would be mutually exclusive from the default being applied, even though if in that future the structure was explicitly set to what is default now and would still work just fine.... this is all 'incorrect'???? and in order to FIX A PAIN, you require this PR change the structure for your opinion as to a 'correct' structure even though you want to implement a future feature that lets user have what ever structure they want, including what the default is today, you wont accept this PR??? goodbye |
@phestermcs Just make Edit: You seemed to miss the fact that I'm not avoiding your PR, I'm trying to do one breaking change instead of two. |
Apologies, but I've gotten a bit lost in this thread. I am one of the (apparently unusual) people who DO need to use both I'll be interested to see where things ultimately land, after PR's are merged and future releases come out. But for the present, can someone please provide some guidance (or an example) for creating a project structure that allows different TypeScript files to employ different definitions? I've experimented with creating separate subdirectories ( |
@steve-perkins I just released 1.0, it may become a little easier for you because you do something like: {
"resolution": {
"main": "main/typings",
"browser": "renderer/typings"
}
} |
i don't have read all the posts above: but is it possible with the following configuration to use only browser? {
"resolution": "browser"
} think its much simpler, i case you want to use the default paths |
@cebor No. You can't, because obviously it's obscure whether that string is a path or a resolution. Do: {
"resolution": {
"browser": "typings"
}
} |
1.x fixed it for me - thanks @blakeembrey |
@basarat would you mind editing the OP to mention that this is fixed in 1.x? I literally read this entire conversation before I found the fix :( |
@LilRed done 🌹 |
For some drama: typings/typings#151
For a complete newbie to typings just trying to get some unit tests for an ionic project running, what is the final solution to the following error?
|
For help, you should probably open an issue. That error isn't related to the larger issue. It's a TypeScript issue saying that a name (variable) named browser is not set when you've tried to use it. |
Typings 1.x no longer creates
browser.d.ts
(onlyindex.d.ts
which is same module resolution pattern as the oldmain.d.ts
). This decreases the setup time for new devs.More on where do typings install : https://github.com/typings/typings/blob/master/docs/faq.md#where-do-the-type-definitions-install 🌹
Original issue report
Would like to discuss if we truly need
browser.d.ts
andmain.d.ts
. Would like to table the desire to removebrowser.d.ts
at least for the time being.With the default tsconfig, Something like
{}
:exclude: ["typings/browser", "typings/browser.d.ts"]
< not intutive to a newbieexclude: ["typings/main", "typings/main.d.ts"]
< not intutive to a newbieGeneral issues
browser.d.ts
/main.d.ts
anyways and that puts them back in step one.main
andbrowser
side-by-side. And for the user that does manage that they can probably generate thebrowser.d.ts
andmain.d.ts
themselves.I just had a colleague go through and start a new TypeScript project and they struggled till I was brought in 🌹
The text was updated successfully, but these errors were encountered: