Switch from Reify to SWC for ensuring compatability with older browsers. #693
Conversation
ILOVEPIE
added
enhancement
dependencies
ILOVEPIE
force-pushed
the
useSWCForVersionTranslation
branch
from
b7f0fcb
to
c23e16e
Compare
|
@yne does this look OK or do we need to make changes? |
|
to be honest
So no, I won't accept it. |
The problem is older versions of OpenType.js don't work properly. Also, we already discussed all this in #619. Also, esbuild does not support transpiling. It only errors when you use a feature that isn't supported by the target. I did a lot of research into this. Also, old versions of Safari are not the only thing targeted by the compat profile. It also supports old versions of Firefox and Chrome and old browsers like Internet Explorer. While you may think there is no use in supporting older browsers, I think there is a large amount of developers out there who work with legacy code who would appreciate being able to use a modern library. Also, do you know how many modern smart TVs are still running old versions of WebKit? Nearly all of them. So if somebody wanted to use the library in a smart TV app, then they're SOL without this. I'm not suggesting we make this the default version of opentype. I'm just saying it's okay to support older browsers for those who need it. And besides, this allows us to drop reify for a dependency that's actually up to date: SWC. Hell, I'll even take over complete responsibility for maintaining the compat profile if you're that against including the profile. It's not like I'm asking for Netscape Navigator support. @Jolg42 & @Connum should weigh in with their opinions before a decision is made. |
Yeah, that was a mistake. I just saw the word "reify" and removed it. I was pretty tired at the time. |
I'm confused, the esbuild documentation state that
So I would have expected some kind of rational/justification explaining why esbuild was not the chosen solution. I'm not against allowing users to build a "compat" version of opentype.js. I'm against complexifing our codebase/buildflow for that. |
|
also, the increasing number of variability that we have to test may require some re-focus
sum up to 16 possible build profile combination to test maybe low memory and legacy are for the same purpose ? I hope that even if we disagree on this subject you'll still ping me some other times 😀 |
esbuild does transform some syntax but it's a very limited transform and it's not as thorough as SWC. Also SWC does a code optimization pass to make the code smaller and more performant by rearranging elements of the code. Also:
SWC does automatic ponyfill injection. So for example we have usages of map that are not supported in older browsers because map was introduced later on. SWC fixes this automatically by injecting a ponyfill of Array.prototype.map that gets used if the function is not available in the browser. It also only injects the ponyfills needed to target a specific browser version at a minimum. So what it does is it only injects the minimum number of ponyfills needed for that target. |
|
The reason I was talking about the erroring was because you originally only linked the supported flag, which is not the same as the target flag. |
|
I actually like this PR, and as @ILOVEPIE stated, we already had a discussion about it. Getting rid of reify will finally allow us to use modern syntax and language features, such as private class fields and methods. iirc it was also holding us back from switching upgrading to the most recent version of mocha. So it actually is a step to more modern development. SWC is also a modern build tool, and many active projects provide legacy versions (may using a "hail Mary" approach and not even running the tests on them again) so I frankly don't get that argument at all. 😄 I don't really care about increased build times, and the complexity is no really increasing that much. There are still so many projects relying on webpack, babel and a great number of plugins for them... So what we'd have now is already way better than that. Regarding the local eslint rules you mentioned: these are for specific performance improvements that we found made an impact on the library, and there were no existing packages for them. You write them once, nothing to maintain except extending them maybe, they're only used for lining, so I don't really see an issue with that either. I think it's even better to have control over them instead of having to rely on yet another third party package. So I'm very happy to see this happening and I'm all for it! |
| let esbuildTarget; | ||
| { | ||
| //These are the targets for a compatibility build that should work with all browsers in use today, even in embedded devices. | ||
| const compatTargets = { |
There was a problem hiding this comment.
Maybe we could put these into package.json and retrieve them from there, so it will be easier to maintain, as we'll probably forget about this file if we don't have to touch it very often.
|
Furthermore, I strongly believe that built complexity or maintenance never was and never will be the reason why opentype.js development is slow. It's our work and private lifes and the lack of sponsorship. 😉 |
|
If this allows removing the barrier on mocha then I'll be for the best. ✔️ Is the Closure Compiler externs file still relevant once transpiling will work ? Echoing to #675 and the last line of #693 (comment) I think one day we shall have a clear view of what opentype.js vision is: "sleek & modern" or "battery-included works-everywhere " (kind of a mac vs windows) |
package.json
Outdated
| "d:umd": "esbuild --bundle src/opentype.js --outdir=dist --external:fs --external:http --external:https --target=es2018 --format=iife --out-extension:.js=.min.js --global-name=opentype --footer:js=\"(function (root, factory) { if (typeof define === 'function' && define.amd)define(factory); else if (typeof module === 'object' && module.exports)module.exports = factory(); else root.opentype = factory(); }(typeof self !== 'undefined' ? self : this, () => ({...opentype,'default':opentype})));\" --minify --sourcemap", | ||
| "b:esm": "esbuild --bundle src/opentype.js --outdir=dist --external:fs --external:http --external:https --target=es2018 --format=esm --out-extension:.js=.module.js", | ||
| "d:esm": "esbuild --bundle src/opentype.js --outdir=dist --external:fs --external:http --external:https --target=es2018 --format=esm --out-extension:.js=.module.min.js --minify --sourcemap" | ||
| "start": "node esbuild-runner.mjs --input src/opentype.js --output dist/opentype.module.js --externals \"['fs','http','https']\" --target es2015 --module --watch --servedir . --global-name opentype --footer \"(function (root, factory) { if (typeof define === 'function' && define.amd)define(factory); else if (typeof module === 'object' && module.exports)module.exports = factory(); else root.opentype = factory(); })(typeof self !== 'undefined' ? self : this, function(){return ((function(a,b){var keys=Object.keys(b);for(var i=0;i<keys.length;i++)a[keys[i]]=b[keys[i]];return a})({'default':opentype},opentype));});\"", |
There was a problem hiding this comment.
why are fs/http/https back in town ?
the two approaches aren't mutually exclusive. That closure compiler externs file is outdated and needs fixes, it basically tells google closure compiler what our API is so it doesn't get mangled or optimized away when people using closure compiler use opentype.js. Closure compiler is like SWC's less hip but way stronger older brother. I opted for SWC because it's newer but that doesn't mean our users will be using it over closure compiler. |
@ILOVEPIE : I've cloned your branch and re-enabled unit-tests (so without --reify) and it failed. Did you created the PR without running the unit-test ? Or where you using a solution that you didn't commited ? If the later (which I really hope), could you share it because the only solution we had for now was to simply declare Also, I setup SWC from scratch by
and it worked. So what was the motivation for creating all those swc wrapper and why was this wrapper even used on non compat builds ? |
|
There was a few issues with the PR. I'm going to fix them later today. I was pretty tired when I finalized the PR and didn't fully take everything into account. The reason why I have the wrapper is because it gives us finer control over the targeting of the builds and also allows us to specifically enable or disable features of SWC that are causing issues in a specific build etc. The solution for fixing the tests I'm probably going to go with is renaming the source files to .mjs because that should let Mocha run them properly. Unless you want me to make a wrapper that runs mocha (just kidding). Also SWC assumes that it's output will be inside a IIFE or module, which means it's better to apply it before bundling. |
|
Also the reason why it was used on non-compat builds was because that way the code would be ES6 compatible even if new features are added and we use them. For example, the spread operator is not ES6 compatible, but we can use it with SWC and still be ES6 compatible. |
ILOVEPIE
force-pushed
the
useSWCForVersionTranslation
branch
2 times, most recently
from
9972973
to
6fefe3b
Compare
|
I fixed some of the test stuff to work better with modules, but I'm still getting a timeout with the HTTP and HTTPS modules. |
ILOVEPIE
force-pushed
the
useSWCForVersionTranslation
branch
from
6fefe3b
to
95915b4
Compare
|
This also closes #692 as it required reworking the same stuff. |
|
I fixed the issues, @yne if you want to take a look, this PR is now using latest mocha. |
ILOVEPIE
force-pushed
the
useSWCForVersionTranslation
branch
from
95915b4
to
2de408e
Compare
package.json
Outdated
| @@ -61,7 +68,7 @@ | |||
| ], | |||
| "extends": "eslint:recommended", | |||
| "parserOptions": { | |||
| "ecmaVersion": 2018, | |||
| "ecmaVersion": 2023, | |||
There was a problem hiding this comment.
at this point we may as well use "latest" or just nothing (default=latest)
so we won't have to update it again
|
glad we finaly moved to module format, but I think that because of all those renaming, all other PR will now be broken ?
I would really prefere using the official |
I think (or maybe it's more kind of a hope) that git will be clever enough so that they can be easily rebased, however, any added files in a PR and their imports will have to be renamed |
Yes let's hope 🤞 Since #619 did not mention |
|
Just realized by looking at #695 that all HTML page importing |
I wouldn't remove them in 2.0.0. I would remove them in 2.0.1 or 2.1.0. Also, we didn't move to the module format. All I did was rename the existing module file names to .mjs. The thing is though I kept the .module.js option for people who are still depending on that. |
This PR doesn't require that, as opentype.module.js still exists. It's just deprecated now. |
The problem with using TSC is that it doesn't work with JavaScript files. We'd have to translate everything to TypeScript. Also, another thing is that the output, I think, only goes as far back as ES5. |
That's specifically why I used git mv instead of the regular move command. |
it does. i've used it. But you are correct on ES5 part. It wont transpile to older standards. |
I would replace them with a deprecation/migration notice/console.error for 2.0.0 , then remove them in 2.1.0 that's what majors are for, |
Sorry, I think there was a miscommunication. That's exactly what I was saying. But I wouldn't replace them with an error. I would just simply spit out an error to the console if they tried to use it, but still have it work. |
No, there are some compatibility targets listed there that are not ES5 but rather have partial support for ES5. |
Sounds like a plan to me. |
|
sure, the js->mjs renames may even be done in it specific PR that could be merged instantaneously, |
ILOVEPIE
force-pushed
the
useSWCForVersionTranslation
branch
from
2de408e
to
84931e4
Compare
|
I succeed (more or less) to make it run on Firefox 4 (2011) using the following build script "scripts": {
"comp": "tsc --allowJs dist/opentype.js --outFile dist/opentype.compat.js"
}However It lack the following classes
<script src="dist/opentype.compat.js"></script>
<script>
var xhr = new XMLHttpRequest(); // member when no fetch() ? I member.
xhr.open('GET', 'test/fonts/TestHVAROne.otf', true); // python -m http.server it
xhr.responseType = 'arraybuffer';
xhr.onload = function(e) {TEST=opentype.parse(this.mozResponseArrayBuffer)};
xhr.send();
</script>see 9fc86d4 |
|
ILOVEPIE
force-pushed
the
useSWCForVersionTranslation
branch
from
84931e4
to
3946e83
Compare
|
I'll rework it. |
This supports a compat profile that works as far back as safari 8, maybe earlier.
ILOVEPIE
force-pushed
the
useSWCForVersionTranslation
branch
from
3946e83
to
1dfe804
Compare
ILOVEPIE
force-pushed
the
useSWCForVersionTranslation
branch
from
1dfe804
to
bfc8dd4
Compare
Description
Created a utility that runs esbuild and SWC to build the library.
This supports a compat profile that works as far back as safari 8, maybe earlier.
This closes #619
Motivation and Context
The library is limited in compatibility by the number of new features we are using, this PR allows us to keep using those features while dropping reify and also supporting even older browsers.
How Has This Been Tested?
I tested the compat outputs using Browser Stack on multiple old browsers including safari 8 and some ancient versions of chrome. I also tested the other builds using more modern browsers.
Types of changes
Checklist:
npm run testand all tests passed green (including code styling checks).