Explicitly set extension on imported source files#1843
Open
peaBerberian wants to merge 1 commit into
Open
Conversation
|
✅ Automated performance checks have passed on commit DetailsPerformance tests 1st run outputNo significative change in performance for tests:
|
While working on TypeScript updates on another project, I noticed a key central things that I did not consider at all in the RxPlayer: the presence of extensions on `import` statements (e.g. the presence of a `.js` / `.ts` for the imported script). Our exposed non-bundled builds (imported when relying on npm + `import` / `require` statements) do not have them, I looked into why and if it may cause issues. --- Having extensions on import statements is technically not an ECMAScript requirement which leaves the task of "module loading" to the host: https://tc39.es/ecma262/#sec-hostresolveimportedmodule Here, hosts are most likely web browsers, which theoretically support ES6 import natively. Its resolution mechanism is well-defined: https://html.spec.whatwg.org/multipage/webappapis.html#resolve-a-module-specifier TL;DR: it constructs an URL and then rely on that URL. Generally, "extensions" are part of the resource name in an URL and cannot be just omitted. Alternatively Node.js and other JS-adjacent hosts also do not have this "magic extension omission" in them, prefering an explicit path instead. --- Thankfully we are saved by the fact that almost all applications rely on us with a bundler (which generally have that extension magic), or when they do not they use our bundles (which by the fact of them being bundles do not need to do `import` statements, removing the issue.) **So this could also be considered a non-issue**, just a "technically, in niche scenarios, we're not as compatible as we could be". I put an emphasis on that because there's no actual pressure to merge this. --- Still, I wanted to see how we could fix this. TypeScript is not excessively helpful here because it will not by itself determine extensions and has a few caveats linked to it. I nonetheless noted 4 possible solutions: 1. We leave our build as is 2. Our `dist/es2017` and `dist/commonjs` builds are now also bundles (e.g. `hls.js` does this). This removes the extension issue but leave multiple other questions (regarding downleveling ES versions, tree-shaking, compatibility with previous versions etc.). 3. We from now on force the usage of extensions on import statements in the source file (e.g. `import X from "../adaptive/` becomes `import X from "../adaptive/index.ts"`). This would work, with the addition of the relatively new (2024) [`rewriteRelativeImportExtensions`](https://www.typescriptlang.org/tsconfig/#rewriteRelativeImportExtensions) TypeScript option, which has many caveats: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-7.html#path-rewriting-for-relative-paths (This are only about dynamic imports, alias etc., features we do not use for now, but to note). It's nonetheless my favorite for its simplicity in terms of concept: when we import, we just put the path to the file that we actually import. Really straightforward with no cognitive load. 4. The solution historically advised by TypeScript, which is to do like 3, but indicating the destination filename instead of the source (here: `.js` instead of `.ts`, more simply put). The nice thing is that it then means less work for TypeScript (it doesn't have to translate paths) and less caveats so all in all less bug chances. But there are several things I don't appreciate with this way, especially the fact that you have to think about the produced output in source files and the poor compatibility with naive tooling just analyzing path without being aware of some weirdness of TypeScript import semantics. So I chose to start here with 3, that this commit implements.
|
✅ Automated performance checks have passed on commit DetailsPerformance tests 1st run outputNo significative change in performance for tests:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Observation
While working on TypeScript updates on another project, I noticed a key central things that I did not consider at all in the RxPlayer: the presence of extensions on
importstatements (e.g. the presence of a.js/.tsfor the imported script).Our exposed non-bundled builds (imported when relying on npm +
import/requirestatements) do not have them, I looked into why and if it may cause issues.The specs
Having extensions on import statements is technically not an ECMAScript requirement which leaves the task of "module loading" to the host: https://tc39.es/ecma262/#sec-hostresolveimportedmodule
Here, hosts are most likely web browsers, which theoretically support ES6 import natively. Its resolution mechanism is well-defined: https://html.spec.whatwg.org/multipage/webappapis.html#resolve-a-module-specifier
TL;DR: it constructs an URL and then rely on that URL. Generally, "extensions" are part of the resource name in an URL and cannot be just omitted.
Alternatively Node.js and other JS-adjacent hosts also do not have this "magic extension omission" in them, prefering an explicit path instead.
Is it an issue?
Thankfully we are saved by the fact that almost all applications rely on us with a bundler (which generally have that extension magic), or when they do not they use our bundles (which by the fact of them being bundles do not need to do
importstatements, removing the issue.)So this could also be considered a non-issue, just a "technically, in niche scenarios, we're not as compatible as we could be". I put an emphasis on that because there's no actual pressure to merge this.
Possible fixes
Still, I wanted to see how we could fix this.
TypeScript is not excessively helpful here because it will not by itself determine extensions and has a few caveats linked to it.
I nonetheless noted 4 possible solutions:
We leave our build as is
Our
dist/es2017anddist/commonjsbuilds are now also bundles (e.g.hls.jsdoes this). This removes the extension issue but leave multiple other questions (regarding downleveling ES versions, tree-shaking, compatibility with previous versions etc.).We from now on force the usage of extensions on import statements in the source file (e.g.
import X from "../adaptive/becomesimport X from "../adaptive/index.ts").This would work, with the addition of the relatively new (2024)
rewriteRelativeImportExtensionsTypeScript option, which has many caveats: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-7.html#path-rewriting-for-relative-paths(This are only about dynamic imports, alias etc., features we do not use for now, but to note).
It's nonetheless my favorite for its simplicity in terms of concept: when we import, we just put the path to the file that we actually import. Really straightforward with no cognitive load and other TypeScript-compatible tools/runtimes will have no issue exploiting it.
The solution historically advised by TypeScript, which is to do like 3, but indicating the destination filename instead of the source (here:
.jsinstead of.ts, more simply put).The nice thing is that it then means less work for TypeScript (it doesn't have to translate paths) and less caveats so all in all less bug chances.
But there are several things I don't appreciate with this way, especially the fact that you have to think about the produced output in source files and the poor compatibility with naive tooling just analyzing path without being aware of some weirdness of TypeScript import semantics.
Relying on the actual filename in source files also seem to be the popular way now that we have many typescript-compliant run times (see TypeScript's 62342 issue, not linking it directly here because GitHub has annoying features when direct links are written).
So I chose to start here with 3, that this commit implements.