kenchris / urlpattern-polyfill

URLPattern polyfill
https://www.npmjs.com/package/urlpattern-polyfill
MIT License
268 stars 31 forks source link

use private class fields #118

Closed jimmywarting closed 1 year ago

jimmywarting commented 1 year ago

Because hard private class fields have the '#' prefix rather than a soft typescript-flavored 'private' keyword, esbuild can mangle the names to look like 1-2 letter words.

I changed the esbuild build process to include "—minify —target=es2022," and I then compared the size it produced.

As you can see, the name has been shortened.

In addition, they shouldn't have ever been made public in the first place.

I prefer using '#' over the TS private keyword since you can tell right away that this, for example:

          if (this.#isHashPrefix()) {
            this.#changeState(State.HASH, /*skip=*/1);
          } else if (this.#isSearchPrefix()) {
            this.#changeState(State.SEARCH, /*skip=*/1);
            this.#internalResult.hash = '';
          } else {
            this.#changeState(State.PATHNAME, /*skip=*/0);
            this.#internalResult.search = '';
            this.#internalResult.hash = '';
          }

...is utilizing hidden, private class fields. without having to pause over a variable or check the definitions of x, y, and z to see if the private keyword was used. Also, it won't build d.ts documentation for these items and won't need to construct any "@private" definitions, resulting in smaller d.ts files because you wouldn't even be aware that they existed from the outside. "#" is a universally understood token or symbol that denotes that something is private. additionally making it tamper-proof.

closes #116

SanderElias commented 1 year ago

@jimmywarting Thanks for the PR, but changing the target did break node-14 compatibility. As LTS support for node 14 ended only a few weeks ago, I don't think we should be dropping support for it just yet. When we are going to drop V14 support, that will make this a breaking change, and we should up the version number again.

I'm talking with @kenchris, to decide what we are going to to. But perhaps you can make some adjustments so this isn't needed?

jimmywarting commented 1 year ago

Only change needed to keep older support would be to change the target version to whatever you like. It can still utilize # for private stuff and mangle name to short names. The only different would be that it instead would use WeakSet or WeakMap in the compiled version.

However i do think NodeJS v14 should be dropped. in other case i think that it could also be useful to compile a new/extra ESM version with newer syntax. like dist/urlpattern-es2022.js or something

SanderElias commented 1 year ago

@kenchris, What do you think about dropping V14 support? I would not mind that much.

@jimmywarting I think adding back in support for V14 is looking for the ??= things and refactor those to what we had before. I don't mind if the class's privates are a bit less private in old versions node versions.

either way, one of those needs to be resolved before we can merge this.

SanderElias commented 1 year ago

@jimmywarting We are ok with dropping V14 support. (@kenchris and I talked it through, and we did a highly scientific Twitter poll 😄) But it needs some attention in the release notes and readme. Dropping support for a NodeJS version is a breaking change. So we will make this a major release as soon as we have all in place.

jimmywarting commented 1 year ago

Where is this twitter poll if i may ask? curious about the result and what ppl voted.

nvm, found it: https://twitter.com/esosanderelias/status/1660872871550279681