Incremental Typescript adoption? #7791
Conversation
|
Edit: Actually there are a few config issues with Mocha 😄 I will try to fix those on the PR and then I'll reopen. But still curious to hear your thoughts! |
|
Amazing, your If you have too much time though: there are many issues with ACTUAL bugs. Why don't you contribute with something solid? |
|
Whoah, hold on @kungfooman - please be polite. I appreciate any time people spend to contribute to this repo. @liamdon has been extremely helpful over the years and should not be berated for making a proposal. |
|
The TS gives promises, but unfortunately it is very alienating to non-TS users to be forced to write TS code. Many issues comes with it:
It's cool to play and enjoy shiny tools, but reality of modern web development is somewhat bleak, and many projects been reduced to unmanageable mess due to non-engineers trying to engineer on real projects that many rely on. Lets solve real problems here please. And not introduce more simply "for fun". |
|
OK! This is why I asked gently 😄 And my example was hasty, I should have spent more time on it. As I said, it’s your engine, but I just wanted to check in on the current thinking of the team. SuperSplat, the Model Viewer, PlayCanvas React and the Editor API are all written in Typescript, while the engine is not.
I’m sorry, I do have to disagree with this though. Build time type checking absolutely reduces logical bugs, that’s the entire point. Otherwise, why do typed languages exist? I’ve worked on large JavaScript, Typescript, C++ and Java projects and the JS projects are definitely the most brittle without extensive test coverage. It’s not a matter of Typescript being “shiny” (it’s 13 years old, only 1 year younger than WebGL). The only reason to adopt it is to catch bugs earlier. With that said, PlayCanvas is a stable engine that I love dearly, and it does already have a form of typechecking via JSDoc and linting. Thank you for the discussion! |
I've worked on dozens of projects in various languages complexities, and team sizes. TypeScript - makes a developer reliant on type-checking to safeguard from naive and primitives coding bugs, while it does not guard from logical engineering bugs at all. Editor - worked for more than a decade, and when I was actively developing it, it didn't even have tests. And believe me - it is still very robust, and reliable, due to its architecture and the way things are structured - also exceptions tolerant. With the engine, the bugs reported in issues - are not syntax or silly bugs that would be prevented by TS, as developers who work on PRs are paying attention, and code in many cases is simply structured, to help readability and stability. Almost all bugs - are complex behavioral/engineering types of bugs.
Yet, many developers don't know and don't want to know it, as they solve real world challenges with just JS. I do work with TS on various projects. Yet I do not recommend it here. When considering something as major as re-writing whole thing, you have to consider a big picture:
The only place where TS would be really useful, is if it would be used only by the developer who already knows TS, and uses Engine while learning, as it would help them in IDE to see what arguments and value types things accept. And that is only for a small subset of users, while others would definitely not need that, as they are working on projects, and cannot be bothered to forcibly re-learn how to write code. |
|
You both make valid points and regarding type checking (build time + Intellisense) and auto complete: under the hood it's the same typing system, the same LSP etc. And I also believe that the types can be better, but it's a slow process. I invite you to run |
|
We've discussed it few times internally, and will again, as that is definitely an important topic. So we'll be discussing it internally, and watching your PR to come back to life with large interest. There is this proposal: github.com/tc39/proposal-type-annotations which we were really looking forward to, but it does not seem there has been much progress on this, so not sure if we'll get this in the future. All of our new tools / repos we have are written in TS, and maybe it's time we consider this for the engine again, especially if there is a way to do this piece by piece with no (significant) downsides. |
…a .js extension for minimal repo changes
|
@mvaligursky OK in that case, here is an improved approach.
|
|
Hi @liamdon , Firstly, thanks so much for taking the time to make this suggestion. I agree with @mvaligursky that it would be awesome to add types to the internals of the engine. It seems a natural extension of the public API types we already have. Besides the compile-time validation, I find the ts syntax is much neater than the equivalent jsdoc. I also think typescript tooling has reached maturity, lowering the risk to us of such a change. On the other hand, the entire team is working flat out on very high priority items and unfortunately we don't have the bandwidth to prepare the parts that would be needed to support piecemeal .ts conversion. This includes things like documentation tooling, examples browser, ci, etc. So yeah, even though we would love to jump on this, unfortunately we can't right now. I'm glad you brought this up and got the whole team thinking - so thanks again! |
|
@slimbuck totally understand! Appreciate you all thinking about it, I will close this PR for now. I'm going to privately continue a few experiments with the repo setup to support some of the use cases you mentioned (eg docs). If the team has bandwidth in future and wants to tackle this, feel free to mention me in an issue and I'll be happy to contribute some code or pointers learned from my experiments. Thanks again! |
|
We do not have an issue on this .. feel free to create one @liamdon and link to this PR, that'd be awesome. |
|
Have you thought through about the drawback to other developers by TS? |
It's probably useful info that 100% of the PlayCanvas team are pro-TS. Today, these repos are TS:
...plus several others. All our new repos use TS (unless there's a very good reason not to). If we were to start the engine from scratch today, it would be TS, I am certain of that. So of course, we have carefully considered the pros and cons. But we (i.e. all PlayCanvas engineers) believe TS is a net win, particularly for larger projects. |
|
Most of the listed repos, are not massively used by other developers as the engine is. So please, first consider the drawbacks for majority of all users. If you will force the TS on users, then please consider of forcing a mandatory poll within the editor on the topic, to see the actual users replies. Also, take a note that three.js being more popular engine, for reasons did not go for TS. This will reduce productivity for many developers massively, and slow down development times as well as reducing dynamic capabilities of JS. The playcanvas team been unable to fix seemingly simple UX bugs in Editor for many months and sometimes years. Whole ESM thing is a huge mess and a massive effort (more than a year?) without any reasonable benefit for users productivity. You sure the team is capable of going to TS? I believe that effort will not only mess up the whole engine, but also will likely kill it for many existing users. |
|
I'm not sure how the engine being implemented in TS would affect any users of the engine? The published package would be identical to now. So we wouldn't force anything on anyone. It's the engine maintainers and contributors that would be impacted (in a good way ;) ) |
|
Recently, I'm maintaining and contributing WebXR APIs integration into the engine. I will not be affected "in a good way" by forced TS into the codebase. This will not make easier for hot-patching the engine to mitigate recently more common breaking changes. Nor it will help to catch and debug engine as compiled version of the engine will not be the same as the source code. Have you worked on commercial projects that forced large TS library on your JS codebase before? If TS would be just the "sunshine", wouldn't the whole industry switch to it already? Does not look like it. So please, look deeper at reasons why it is not such a great idea as it seems. |
Yes if we had the opportunity to move to TS you would be forced to add types to your internal variables as well. You already added jsdoc types to all public API. Is the extra burden really what you're complaining about?
Are you hot patching and running directly from the source tree?
The difference between TS source and esm version is extremely manageable. I would say TS output is probable more similar to source than esm output to cjs versions we already generate. We can deal with all this thanks to source maps and maturing tooling.
Are you suggesting that large commercial projects are developed against the original engine source tree? I assume that large commercial projects use the published, tested and supported version of the engine for development, perhaps patching critical issues occasionally. But even then, not against the source tree. I hope my responses don't come across as argumentative @Maksims. I respect you as an engineer and would love to understand the precise issue you have with typescript. |
Lets look at an actual example when @kpal81xd killed ESM import from
(... count the But now it's called "TypeScript" and you really believe it's type safe? Lets look at some old issues I opened: What did you @mvaligursky say about standalone repros that are quick and easy to fix via
And now you are bloviating about "no significant downsides" as if the TypeScript Gospel has hit you on the head. The precise issue is TypeScript's cumbersomeness. You cannot just simply use the source-of-truth any longer, you have to check out a PR, spawn a shell, type this and that to build it, wait for the building process, hope you get the right import from Meanwhile my actual type improvements in JSDoc where constantly shot down by @willeastcott - you would find any argument to not improve types. And you pretend that I'm berating. Look in the mirror. |
There are bright bits of course. But you don't know how many times I've struggled working in this code base without types. Day after day. It's hard. And how many times I shipped with issues that would be impossible in typed codebase. It's all about balancing all these pros and cons. |
Yes, I struggle too without types and I love IntelliSense and I don't understand your "But". This word is indicative of dismissing everything said before and it simply doesn't make sense - I just pointed it out. The "PlayCanvas TypeScript conversion process" is renaming |
|
@kungfooman I'm going to ignore your straw man arguments. Please stop. In summary it seems to me that you consider the ability to directly include the source tree files in the browser to be more important than anything else. This is the superpower that means you can work and debug effectively. You're saying that any single translation step would effectively ruin this ability and cause you endless grief. So basically it actually has nothing to do with typescript itself, but rather the presence of any translation step. Is that a fair summary? |
JSDoc - is easy to write, it does not lead to blocking a developer with prototyping and can be done when PR is being tidied up.
We do various things, depends on the issue. If it is a breaking change we cannot afford spending a day to migrate, then we just compile engine ourselves. The Editor's choice of engines goes only one back. Does not allow to specify specific version. If we found a significant optimization path, then we do hot-patch of live engine in our app, by simply overriding prototype methods - that worked a charm, and allows us to simply go to source code of the engine, copy the code, modify and have it loaded after engine. With TS - this will not be a possibility. PR Merges can be slow (we need to move fast), or some hot-patch might be very specific to us. If it is a bug, we do hot-patch also: copy source method, modify, load after engine. With TS - this will not be a possibility.
The beauty of cjs compile engine, is that you can still use raw code from git, as it is, if you are ok for not supporting outdated browsers. Because the current source code runs directly in the browser, just grab'n'go.
We have various cases, where we have to do some custom engine code, like custom components, or hot patching, whatever. The beauty is that we can simply import the current engine directly as mjs, and have our own bundling or even without. As developer see fit. Because currently engine does not force an opinions onto developer, and while providing default building path, it can run directly in the browser without building.
My apologies for me sounding harsh. I've been working on PlayCanvas as a team member from earliest days, and now I do a lot of work using PlayCanvas as an agency, I have many developers and creatives, as well as working with other agencies who make their business fully based on PlayCanvas. |
| async function handleRequest(request, response, serveConfig, rootDir) { | ||
| const parsedUrl = parseUrl(request.url ?? ''); | ||
| const requestPath = parsedUrl.pathname; | ||
|
|
||
| // Only intercept .js file requests | ||
| if (requestPath?.endsWith('.js')) { | ||
| const absolutePath = path.join(rootDir, requestPath); | ||
| const jsExists = await fileExists(absolutePath); | ||
|
|
||
| // If .js file doesn't exist, check for .ts file | ||
| if (!jsExists) { | ||
| const tsPath = absolutePath.replace(/\.js$/, '.ts'); | ||
| const tsExists = await fileExists(tsPath); | ||
|
|
||
| if (tsExists) { | ||
| try { | ||
| const transpiledCode = await transpileTypeScript(tsPath, absolutePath); | ||
|
|
||
| // Send the transpiled JavaScript | ||
| response.setHeader('Content-Type', 'application/javascript; charset=utf-8'); | ||
| response.setHeader('Cache-Control', 'no-cache'); | ||
| response.statusCode = 200; | ||
| response.end(transpiledCode); | ||
| return; | ||
| } catch (error) { | ||
| // Send error response | ||
| response.setHeader('Content-Type', 'text/plain; charset=utf-8'); | ||
| response.statusCode = 500; | ||
| response.end(`Error transpiling TypeScript file: ${error.message}`); | ||
| return; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Ideally we would not want any custom code to auto-convert the typescript files to JS - The serve NPM script would serve the final built version of the engine which would already be in JS so this conversion would not be needed at this stage
There was a problem hiding this comment.
Yeah, I generally agree - but then we have this use-case described in the README where you can use the engine source directly:
ENGINE_PATH=../src/index.js npm run develop
I figured this might be a common use-case for engine developers who are working on features and want to preview them live. And this feels like the kind of thing Maksims is concerned about, being able to quickly edit and preview the live engine code.
However, what we should probably do is serve the built version normally with no transformation unless they have specified the raw source ENGINE_PATH, in which case the above could still apply. We could basically check ENGINE_PATH in the handler above and early exit with the serve unless it's ../src/. We are still using serve, it's just the library version rather than the CLI.
This development case is where people often reach for Vite as an extension to Rollup, the dev server will do all the transformation for you. But there is already a heavy investment in Rollup plugins and we don't need most Vite features, so that felt like too large a change.
There was a problem hiding this comment.
Yea thats the only place thats its used directly - It would be better to use tsc watch to chain from the source directly instead of transformation during serving. It would be just as fast it just needs to be setup correctly.
Yea I want to use vite ngl but its the fact that we have multiple sources and outputs thats makes it difficult to migrate but vite has some nice caching and hot reloading so its something I have considered.
| it('correctly registers an ESM script with its scriptName', function () { | ||
| class TestScript extends Script { | ||
| static scriptName = 'myTestScript'; | ||
| static __name = 'myTestScript'; |
There was a problem hiding this comment.
This is a PlayCanvas script specific field for identifying the scriptName when adding to a component so should not be changed
There was a problem hiding this comment.
Got it, I will find a workaround. PlayCanvas really didn't like that scriptName is a getter with no setter and we are assigning it here, but apparently JS/JSDoc still lets you do this.
There was a problem hiding this comment.
That should just be an eslint rule we can update for scripts so shouldn't be an issue.
| /** | ||
| * Rollup plugin to resolve .js imports to .ts files | ||
| * This allows TypeScript files to be imported with .js extensions | ||
| * | ||
| * @returns {import('rollup').Plugin} - The Rollup plugin. | ||
| */ | ||
| export function resolveTsExtensions() { | ||
| return { | ||
| name: 'resolve-ts-extensions', | ||
| resolveId(source, importer) { | ||
| // Only handle relative imports that end with .js | ||
| if (!source.startsWith('.') || !source.endsWith('.js')) { | ||
| return null; | ||
| } | ||
|
|
||
| if (!importer) { | ||
| return null; | ||
| } | ||
|
|
||
| // Resolve the full path | ||
| const importerDir = dirname(importer); | ||
| const fullPath = resolve(importerDir, source); | ||
|
|
||
| // Try replacing .js with .ts | ||
| const tsPath = fullPath.replace(/\.js$/, '.ts'); | ||
|
|
||
| if (existsSync(tsPath)) { | ||
| return tsPath; | ||
| } | ||
|
|
||
| // Let other plugins handle it | ||
| return null; | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
So ideally we would not want custom code to resolve the import mapping to allow for js and ts - we would perform the minimum amount of work to convert all the JS files to TS and then over time we would narrow the types down
There was a problem hiding this comment.
I'm open to that - I did this after the feedback about import maps etc. Basically assuming that we will be doing an incremental file-by-file migration, so for a period of time we need to support .ts and .js alongside each other, and we want to preserve all the imports in the meantime. This is what allows other files to continue doing import { Color } from "./color.js without knowing that it's a TS file, so that limits the blast radius of single migration.
Are you saying that it would be better to one-shot the conversion across all files, probably with least strict tsconfig.json, and then improve those types over time before making the typechecking stricter? If so, I'm certainly not opposed, but I could imagine the PlayCanvas team being worried about the safety and testability of such a massive PR.
There was a problem hiding this comment.
Yea when I mean one shot itll be the same as for the observer that I did a while ago where the main task was doing the minimal amount of work to convert to TS and then incrementally update it after. This eliminates potential build issues and complexity and through custom plugins.
Yes it would be a massive PR but if you automate the conversion process then the risk would be minimal.
There was a problem hiding this comment.
Oh nice, I will study your PR on that repo. Did you use a tool to automate the conversion for that?
There was a problem hiding this comment.
So this one I made some scripts to help but nothing major. For the engine it's much larger so I'll definitely look into something more robust.
|
Try converting Entity - that would give you a better idea of the whole conversion troubles. Color - is one of the most simple class and does not represent how complex it will be. This will take resources away from implementing real features. |
Absolutely. A main reason we cannot afford to do this in the near future. To do this properly would be a large job. And we would not want to have the code base in mixed state for very long or indefinitely. |
Thank you for the summary @slimbuck, you got it. The "endless grief" sounds of course like hell, in practise it would be wasted time and nerves for nothing.
Right, I had my escapades writing TypeScript for many years. The deeper I digged into TS, the more fancy type-programming constructs I would add to my code bases and if you do that long enough, at some point you realize: I can't read that mess any longer (or at least not without a premium amount of concentration and time). But you also can't prevent it if you are "really serious" about type safety. A good recent example from here: #7560 from @willeastcott and we can all guess why it became stuck (tldr it's not a simple type problem). If you want the "proper" types, you suddenly require constructs like: It becomes a mental task to discern JS from TS in a TypeScript codebase. What is fluff, what is "real". It becomes an unholy tangled mess. So funnily enough at some point it dawned on me that I can have both worlds: readable JavaScript and the "type mess" can be easier discerned being encapsulated inside JSDoc comments. TypeScript beginners will not run into this problem until they learn advanced type programming, everything is just
Yes, thank you for that. 👍 |
|
Even this PR already starts to do some funky TS shenanigans: const cstr = this.constructor as new (r: number, g: number, b: number, a: number) => this;Or: toArray<T extends ArrayLike<number> & { [key: number]: number } = number[]>(arr?: T, offset: number = 0, alpha: boolean = true): T {
const result = arr || [] as unknown as T;If you want allow people to make PR's, and force them to decipher this, well, it will likely reduce potential PR makers all the way down to 0. Wait till you have to make typing into |
|
Thanks @kungfooman and @Maksims. This is the discussion I was hoping for! I agree that typing parts of the pc engine will likely be unwieldy. I guess converting existing codebases to typescript could be more fraught than writing the code in typescript from the get go. If you write your code in typescript upfront, you will presumably structure the types to conform correctly. Doing so after the fact could be challenging. (Though I still think it's worth us trying should we ever have the time).
I'm not going to disagree with you that this code isn't as readable as before :) I have actually struggled with managing constructors and types themselves in the past. My assumption is the typescript guys are working on that. On the other hand, if you consider this from the PR: vs We've lost some documentation on the r component, but overall it's plain that the typescript version is much more succinct and readable than the js/doc equivalent. And I think this type of change would account for the vast majority of updates in the engine. |
|
@slimbuck It only looks shorter because he broke the docs, something @willeastcott has specifically decided to add and invest time in: #7570 So if this were a proper TS rewrite, it should look something like this: /**
* Creates a new Color instance from an array.
*
* @param arr - An array of 3 or 4 numbers representing [r, g, b, a]. The alpha (a) defaults to 1 if not provided.
* @example
* const c = new Color([0.1, 0.2, 0.3, 0.4]); // r=0.1, g=0.2, b=0.3, a=0.4
* const c2 = new Color([0.1, 0.2, 0.3]); // r=0.1, g=0.2, b=0.3, a=1
*/
constructor(arr: number[]);
/**
* Creates a new Color instance from individual components.
*
* @param r - The red component. Defaults to 0.
* @param g - The green component. Defaults to 0.
* @param b - The blue component. Defaults to 0.
* @param a - The alpha component. Defaults to 1.
* @example
* const c1 = new Color(); // r=0, g=0, b=0, a=1
* const c2 = new Color(0.1, 0.2, 0.3, 0.4); // r=0.1, g=0.2, b=0.3, a=0.4
*/
constructor(r?: number, g?: number, b?: number, a?: number);
constructor(r: number | number[] = 0, g: number = 0, b: number = 0, a: number = 1) {
...
}You don't need these lines in JS, that is what the constructor(arr: number[]);
constructor(r?: number, g?: number, b?: number, a?: number);Anyway, even if this would have been a properly rewritten PR, breaking ESM for a superficial change would be an absolute no-go for me. It has already estranged me from all other PC repos and I just hope this is never going to happen for the "heart" of PlayCanvas. |
7 participants
Note: this is an "asking a question" PR, not a direct merge proposal.
What is the PlayCanvas team's position on Typescript these days?
I'm curious whether you'd be open to incrementally migrating the PlayCanvas codebase to Typescript over time, or whether this is a non-starter for the team.
Demo: migrate pc.Color to Typescript
I noticed that it's easily possible to mix both Typescript and Javascript files in the engine source. You just need a very minimal change to the rollup config, and you need to drop the file extension when importing a Typescript source file. I migrated color.js to Typescript to demonstrate.
With only these changes, everything works seamlessly - you can import JS from TS and vice-versa, all Intellisense features work as expected, and the engine build output works with correct types as before (all examples work). I was surprised at how easy it was to mix both TS and JS in the same codebase like this!
What do you think?
If you were open to this direction, you could start gradually migrating existing and new files over to Typescript. With AI tools, a straightforward translation like this is not half as daunting as it used to be.
I know you've put a lot of work into the build system and into TSDoc annotations, and for the most part those are great. But more advanced type inference is much easier when you're using Typescript natively, and this could be used, for example, to infer types on things like
asset.resource. Right now this is typed asobject, but it could automatically be inferred aspc.Texturewhen the asset is known to have atypeof"texture"!Anyway, this is not a heavy push from me - it's your engine - but I'd love to know if you have a position on this, and if so, whether you'd be open to using an incremental migration strategy as demonstrated in this PR.
I confirm I have read the contributing guidelines and signed the Contributor License Agreement.