kenchris / urlpattern-polyfill

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

Patch the global object in ESM mode #71

Closed kenchris closed 2 years ago

kenchris commented 2 years ago
kenchris commented 2 years ago

Not sure if there is a better approach. @abdonrd @justinfagnani

abdonrd commented 2 years ago

Hmmm with this PR you can't import just the implementation.

The /dist/index.js do the global patch: https://unpkg.com/browse/urlpattern-polyfill@3.0.0/dist/index.js

And you can't import the /dist/index.impl.js because it's not in the package.json exports: https://github.com/kenchris/urlpattern-polyfill/blob/2764da6664a58b660b5e3314f0570b2a09d5728e/package.json#L10-L14

abdonrd commented 2 years ago

Also the esbuild dependency is missing from the package.json.

abdonrd commented 2 years ago

Also, why not expose the library just as ESM?

Compile the TS to JS and that's it.

SanderElias commented 2 years ago

@abdonrd , may I ask why you want to get acces to the implementation? This is a polyfill, not a generic user land library.

We can't only expose esm, because that might conflict with nodejs commonjs usage. Commonjs is sync, while esm modules are async by default. I have a PR in flight that does address parts of your ask, and if there is a reason to expose the implementation i can add this

abdonrd commented 2 years ago

@abdonrd , may I ask why you want to get acces to the implementation?

To let the application manage the loading. Reference: https://github.com/kenchris/urlpattern-polyfill/pull/71#discussion_r840073586

For example with: https://modern-web.dev/docs/building/polyfills-loader/

Edit: Well, accessing the implementation is not necessary if the main file is something like:

import { URLPattern } from './url-pattern.js';
globalThis.URLPattern = URLPattern;

In this way, with the application deciding if it wants to import the polyfill or not would be enough.

abdonrd commented 2 years ago

We can't only expose esm, because that might conflict with nodejs commonjs usage. Commonjs is sync, while esm modules are async by default. I have a PR in flight that does address parts of your ask, and if there is a reason to expose the implementation i can add this

I think we can do something like this: https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c

Just require some Node versions: "node": "^12.20.0 || ^14.13.1 || >=16.0.0"

abdonrd commented 2 years ago

And I guess typos would be necessary for cases like this?

https://github.com/lit/lit/blob/c41a92c96eeb8c2db4875c94c2eabcd512e044c4/packages/labs/router/src/routes.ts#L8-L11