js-temporal / temporal-polyfill

Polyfill for Temporal (under construction)
ISC License
528 stars 28 forks source link

Export already packaged lib folder #283

Closed espretto closed 5 months ago

espretto commented 6 months ago

Hello 👋 , the lib folder is part of the npm package as configured in the package.json field files. However it is not accessible unless listed in the exports field. Currently I have to override my bundlers dependency resolution to access the folder's contents.

12wrigja commented 6 months ago

We originally include our sources in the bundle so they can be referenced using source maps when debugging, not to be consumed directly by bundlers.

Can you help me understand why you can't apply the transpilation to the relevant bundled ESM in dist, which is the intended artifact to consume? My recollection is that running the Babel transform for JSBI -> native bigint over that file will get you want you want (based on my previous comments here), though the thread there implies that configuring webpack in practice (vs my toy example) is non-trivial.

espretto commented 6 months ago

I didn't realize the ts sources where included for debugging purposes. Makes sense.

Also, I didn't think of using the babel plugin on the ESM build but yes, that should definitely work. Using ESM modules however, is still a little tricky to configure when targeting non-ESM environments.

My intention is to create a partial polyfill by picking only those parts of the polyfill that I need in my codebase. Since tree-shaking is not really an option w/ Temporal's OOP design and its cross-referencing conversion methods, this approach seems to hold the best balance between flexibility/bundle-size and spec compliance.

I am aware that this is branching out quite far from where I started.

espretto commented 6 months ago

Here is what my partial polyfill looks like. This would become possible if the ./lib folder were accessible.

import * as PlainDateLib from "@js-temporal/polyfill/lib/plaindate";

export namespace Temporal {
  /** @see https://www.typescriptlang.org/docs/handbook/namespaces.html#aliases */
  export import PlainDate = PlainDateLib.PlainDate;
}

Of course I'll have to setup my toolchain to be compatible w/ the sourcecode.

12wrigja commented 6 months ago

My intention is to create a partial polyfill by picking only those parts of the polyfill that I need in my codebase. Since tree-shaking is not really an option w/ Temporal's OOP design and its cross-referencing conversion methods, this approach seems to hold the best balance between flexibility/bundle-size and spec compliance.

The intent seems reasonable, but doing this by depending on a released artifact seems like the wrong approach. Instead, given you are effectively forking the polyfill, I would suggest you depend on our repository directly using a git URL dependency: https://docs.npmjs.com/cli/v10/configuring-npm/package-json#dependencies

Note that we give absolutely no guarantees about the content of our repository.

espretto commented 5 months ago

I understand that you wouldn't want to support this approach as the official way of doing things. I'd have to try if using a git-based npm dependency is compatible w/ our build-pipeline and/or if adding the repo as a git submodule would be a viable solution (though I doubt it).

UPDATE: I am now using npm package patch-package to apply this change as part of my postinstall routine. Closing this PR.

12wrigja commented 5 months ago

UPDATE: I am now using npm package patch-package to apply this change as part of my postinstall routine. Closing this PR.

Are you able / willing to link to that on GitHub (or is it closed source)? It might be a good example for others who may want to do the same thing.

Furthermore, I'm wondering if you've run into any issues relying on the implementation this way as a result of the implicit cyclic dependencies / intrinsic class registrations and usage in various class methods that transform one Temporal type to another (e.g. https://github.com/js-temporal/temporal-polyfill/blob/44ea889133e0da5b062bbc7660ed77d995430d90/lib/plaindatetime.ts#L388 - I'm wondering if this fails at runtime because ZonedDateTime was never evaluated and so trying to lookup ZonedDateTime at runtime will fail)?