Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FR: provide a "modern" ESM build without jsbi #155

Open
taralx opened this issue May 12, 2022 · 36 comments
Open

FR: provide a "modern" ESM build without jsbi #155

taralx opened this issue May 12, 2022 · 36 comments

Comments

@taralx
Copy link

taralx commented May 12, 2022

There's a babel plug-in for it, but it would be nice to have a ready-to-import version.

@12wrigja
Copy link
Contributor

#138 (comment) seems related.

For your use-case, are you planning on using this polyfill:

  • only within applications (that don't themselves become NPM packages) and can made the decision whether they need JSBI
  • only within an ecosystem of packages where native BigInt is considered available on the platform?

Are you aware of any other packages that have this sort of a problem and how they went about solving it? I'm not opposed to having a solution, but I'd prefer not to create a bifurcation in the ecosystem where there are some packages using this dependency assuming that native BigInt is available only to find that someone who wants to depend on those needs JSBI.

Would it work for your use-case to have JSBI defer to native BigInt if available, but still pay the code-size cost of JSBI itself? This used to be a feature in JSBI but was removed.

@taralx
Copy link
Author

taralx commented May 12, 2022

I'm using it on browser, where we expect all users to have bigint (we need newer features anyway). Having it defer would be ok, but we're trying to keep download sizes down as well, preferably without introducing an additional transpile step.

@taralx
Copy link
Author

taralx commented May 12, 2022

If you prefer not to create a third output (.bigint.mjs), then what about just applying this to the ESM output? Anyone who is worried about older platforms like that is going to be using CJS anyway.

@12wrigja
Copy link
Contributor

I'd probably lean towards making more outputs that are listed as conditional exports (https://nodejs.org/api/packages.html#conditional-exports) and could be imported like so:

import {Temporal} from '@js-temporal/polyfill/bigint';

Or some such.

I don't want to tie this to the module system used as it can be quite surprising which tools chose to use what thing we specify in package.json (see #29 for example).

@12wrigja
Copy link
Contributor

Though I think this strategy will lead to weird bifurcation issues within the ecosystem and likely weird issues if both versions are in use at once (besides the expected doubled code-size).

@12wrigja
Copy link
Contributor

trying to keep download sizes down as well, preferably without introducing an additional transpile step.

To be clear: have you tried the Babel plugin and does it meet your needs besides having another build step?

@taralx
Copy link
Author

taralx commented May 12, 2022

I haven't tried it but it almost certainly works. It's just that right now we don't have a transpile requirement, and I was hoping to keep it that way. 🙂

@12wrigja
Copy link
Contributor

Would you mind describing your current dev & prod build setups in more detail? I'd like to understand why adding a transpilation step here is something you want to avoid. I can generally understand the desire to have fewer tools/processes in place, but any general solution to the bifurcation problem I presented above likely requires ecosystem-wide tooling support. If your goal is to reduce download code-size then I'd have thought you would already have transpilation / minification in place for prod builds.

@taralx
Copy link
Author

taralx commented May 13, 2022

Right now it's all just direct <script> tags from unpkg. No build steps at all.

@taralx
Copy link
Author

taralx commented May 13, 2022

You're right, we're probably going to need to set up a build at some point. I was just thinking maybe others might want the same thing here. But I also understand the fragmentation / too many artifacts argument.

@12wrigja
Copy link
Contributor

Thanks for your understanding. It's less about the number of artifacts and more about fragmentation / confusing problems stemming from using two copies of the Temporal source simultaneously.

That being said, this discussion is making me reconsider attempting upstream adjustments for JSBI to use native BigInt where available so that whilst code-size might suffer more the runtime performance should improve for most users.

@jimmywarting
Copy link

👍 would also want a scaled down version. don't care about IE anymore. all the new browser have auto updates and have had BigInt for a while now.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt#browser_compatibility

@12wrigja
Copy link
Contributor

12wrigja commented Jun 2, 2022

webpack/webpack#15903

I've filed this issue against Webpack to consider what it would take to have support in bundler tools for the sorts of conditions we would need to make this "automatic".

If we generated a build that only ever uses native bigint and referenced it in package.json using a custom condition, would that be enough? I'm concerned that won't be compatible with the various build tools that people use, and that configuring it would be just as difficult as installing and using the JSBI Babel plugin).

If something like this ends up working, perhaps we can flip the defaults (so by default users use a build that uses native bigints, and you have to opt in to using JSBI if your build needs it).

@taralx
Copy link
Author

taralx commented Jun 3, 2022

That's a great idea, but probably needs to be up a level at the package manager, i.e. npm/yarn/etc. rather than at the bundler level.

@12wrigja
Copy link
Contributor

12wrigja commented Jun 3, 2022

I'm not sure I follow why this needs to be moved to the package manager level, given that at the moment only bundlers consume the content in the exports field of package.json anyways?

@taralx
Copy link
Author

taralx commented Jun 4, 2022

I suppose "needs to" is too much, but "would get better traction" in my opinion. Individual bundlers mostly try to follow node.js package.json conventions unless a compelling case exists for deviating from it.

@adidahiya
Copy link

👍 would also want a scaled down version. don't care about IE anymore. all the new browser have auto updates and have had BigInt for a while now.

Same here. I want to use the temporal-polyfill library in a UI toolkit where I don't care about IE support anymore. I use TypeScript to build/transpile my project, and I really don't want to add a Babel dependency to my build system.

@alippai
Copy link

alippai commented Jun 14, 2022

Having the transpile step can make development slow, eg. what if someone uses parcel? Adding babel would be noticeable, running the transpile on every compile sounds unnecessary.

@12wrigja
Copy link
Contributor

12wrigja commented Jul 7, 2022

Adding babel would be noticeable, running the transpile on every compile sounds unnecessary.

Generally this shouldn't be necessary - you could instead only run this in a build destined for production, where you might also be using an optimizer/minifier and other more expensive build steps.

My current thoughts on this: if two versions of this polyfill were to play nice-ish together when combined at runtime then I could see having either another named export for "Temporal with native BigInt", or another npm package as then if they were both used within a project's transitive closure Temporal would at least work (sans code-size issues that stem from having two versions of a package in a build). I'm tracking investigation on that over in #164, but it's not a high priority for me and community contribution to fix this issue would be appreciated.

@celluj34
Copy link

I would also be interested in this - I'm aware of the babel plugin but we're not using babel at all currently, so having to include that just for one plugin seems excessive.

@rjgotten
Copy link

rjgotten commented Jul 27, 2023

Some food for thought:

Not offering a ready-to-go version and telling people to use an experimental Babel plugin to strip JSBI is basically a big middle finger to every React app built using the basic create-react-app flow.

CRA doesn't allow for modifying the underlying Webpack or Babel configurations without 'ejecting' (forcing those people to take on full ownership of the configuration of everything ); forking the basic templates and maintaining their own incl. keeping it up-to-date with upstream (again: forcing those people to take on full ownership); or relying on unsupported community hacks that try to open up the configuration piecemeal - but which are unsupported and in some cases actively frowned upon by the CRA maintainers.

For these cases, it's not a matter of 'just add Babel' but literally a matter of 'can't add Babel' - at least: not without taking on a disproportionately large additional maintenance burden.

@Pyrolistical
Copy link

Pyrolistical commented Sep 24, 2023

I did a quick hack to minimally make a build without jsbi: Pyrolistical@92c895b

To use it run npm run build:hackout-jsbi

You can see the full command in package.json

Result:

$ node
Welcome to Node.js v20.6.1.
Type ".help" for more information.
> const { Temporal } = await import("./dist/index.js");
undefined
> Temporal.Now.instant().toString()
'2023-09-24T20:09:47.968182747Z'
> Temporal.Now.instant().epochNanoseconds
1695586189666187968n

You can vendor the dist as temporal into your own project.

And for typescript users, to get types, copy index.d.ts and enable "allowJs": true in tsconfig.json

@12wrigja
Copy link
Contributor

12wrigja commented Nov 21, 2023

Some recent thoughts I've had:

  • It makes sense to me that the default published build for this project does not use JSBI. However, the project source should continue to.
  • For any users that are trying to ship this to older browsers, they might should be able to:
    • use NPM's overrides mechanism to replace this package in their entire transitive closure with a version that uses JSBI at runtime. Edit: this might be made easier using a tool to manage it (e.g. https://github.com/rogeriochaves/npm-force-resolutions)
    • how they get this version is still up for debate - maybe we could publish two packages, one with JSBI and one without? (@js-temporal/polyfill-bigint-compat)?

We (project maintainers) would need to adopt a slightly more complex workflow (because the modern package we publish should have no deps, and would no longer match the package.json we have checked into VCS), but that seems fairly simple.

That should shift the disproportionate maintenance burden to the people who cause it (need older browser support), and provide sensible modern defaults.

I don't have the time right now to actually set this up, but did want to float the idea here to see if anyone has objections / known edge-cases that this wouldn't address.

@jimmywarting
Copy link

I have always been a bit against the way package.json can magically change the import behaviour. i think package.json was a misstake from begin with. conditional import (in javascript) and only polyfill stuff when necessary is the way to go imo.

globalThis.DOMException || await import('https://jscdn.xyz/DOMException/polyfill.js')

I always try to create solution that works in a way such that if you had no information about a package.json, what runtime you are using or how any of the new tomorrow bundler will work and behave (cuz they might not take any consideration into any overrides) or knowing what target you are building for

Try to think of it like if deno wanted to do a http-import and did not have any support for bigint or any package.json information.

@rjgotten
Copy link

rjgotten commented Nov 22, 2023

@jimmywarting
BigInt can't be polyfilled because it is in part a language feature, and not an API.
It has problems with fallbacks being transpiled in as well, because essentially any piece of math no matter how trivial might at one point need to operate on a BigInt.

This is why solutions like JSBI took the approach that you have to author your code itself with the bespoke syntax of the fallback library; and then you have a transpiler plugin to recognize that library-specific syntax and pull it back out.

The discussion here is about the fact that not everyone has access to a transpiler in their specific setup. (Or has access to configure the transpiler. See for instance the situation with create-react-app and the need to 'eject' the configuration.)

It's about recognizing that there is value in supplying a ready-to-go distribution with JSBI already removed.
And about finding a way to do that.

@jimmywarting
Copy link

jimmywarting commented Nov 22, 2023

i didn't try to make my comment specific to just JSBI and bigints.
i only wanted touch the thing with package.json

if it's the case that BigInt can't be polyfilled then it should do:

temporal = globalThis.temporal || await import(typeof BigInt === 'undefined' ? './temporal-jsbi.js' : './temporal-bigint.js')

rather than relying on package.json overrides. Some day react-native might have support for bigint. and it is beter to do feature detection instead.

@rjgotten
Copy link

That will not work, for the very simple reason that import expressions are asynchronous and a poison pill.
It means any code using Temporal suddenly can only access Temporal from an async context.
Total no-go.

@lionel-rowe
Copy link

That will not work, for the very simple reason that import expressions are asynchronous and a poison pill. It means any code using Temporal suddenly can only access Temporal from an async context. Total no-go.

Not in ESM contexts. As long as the await is at the top level and that module is imported at the app's entrypoint before any other imports, it won't cause problems.

// sync-export.mjs
export const resolved = 1
export const globalVar = 2

// async-export.mjs
export const { resolved, globalVar } = await import('./sync-export.mjs')
globalThis.globalVar = globalVar

// sync-import.mjs
import { resolved } from './async-export.mjs'
console.log(resolved) // 1
console.log(globalThis.globalVar) // 2

v8.dev's Top-level await article explicitly lists dependency fallbacks as a use case:

let jQuery;
try {
  jQuery = await import('https://cdn-a.example.com/jQuery');
} catch {
  jQuery = await import('https://cdn-b.example.com/jQuery');
}

@rjgotten
Copy link

rjgotten commented Dec 11, 2023

Not in ESM contexts.

Which means the requirement placed on developers shifts from "you have to use a toolchain that involves Babel and can be configured to transpile JSBI out" to "you have to use a toolchain that understands top-level awaited modules".

In practice that means you're shifting from "you need to use Webpack" to "you need to use a very recent version of Webpack" which, to say, does not improve matters. It's rather a case of 1 step forward, 2 steps back wrt ease-of-use.

The best way is still the package.json override, because that one is centered on features integral to Yarn and NPM; either of which you HAVE to be using anyway if you're installing the polyfill offered in its packaged format.

@lionel-rowe
Copy link

The best way is still the package.json override, because that one is centered on features integral to Yarn and NPM; either of which you HAVE to be using anyway if you're installing the polyfill offered in its packaged format.

I'm using Deno, importing via an esm.sh URL (same approach also works natively in all modern browsers). No build tools, no package managers, no package.json.

@LinusU
Copy link

LinusU commented Feb 12, 2024

It makes sense to me that the default published build for this project does not use JSBI.

Nice! I think that this is a very welcomed change, since basically every environment supports BigInt natively.

According to caniuse:

Since September 2020, this feature works across the latest devices and major browser versions (Learn more about Baseline)

I believe that it's only Internet Explorer 11 that doesn't support it, which is end-of-life as of 14 Jun 2022.

However, the project source should continue to.

Im curious about the reasoning here, sorry if it's already specified elsewhere in the project, I had a search but couldn't find anything. Is this only to support Internet Explorer, or is there something else that I'm missing?

@12wrigja
Copy link
Contributor

Is this only to support Internet Explorer

Correct - we want to provide back-compat support for projects that need it, even if it requires more complicated project setup on their end. If we swapped to using native bigint in source directly it would be very difficult to "un-do" that locally in a project.

That being said, we don't currently have CI testing on IE11 so I can't confidently say the current implementation even works in IE11 even with transpilation to ES5.

While this is something I want to see done, I'm not sure I have the bandwidth to work on it (my day job has shifted priorities, and helping to maintain this project isn't as high on the to-do list as it was previously). If anyone here is interested in contributing a PR to see it happen I would be happy to review it.

@espretto
Copy link

espretto commented May 7, 2024

Hello 👋
it seems to me the browser build should rely on native BigInt and for back-compat support a separate build should be introduced. Projects that need it can always override their bundler's dependency resolution for specific packages (commonly using "aliases"):

@rjgotten
Copy link

rjgotten commented May 7, 2024

The best way is still the package.json override, because that one is centered on features integral to Yarn and NPM; either of which you HAVE to be using anyway if you're installing the polyfill offered in its packaged format.

I'm using Deno, importing via an esm.sh URL (same approach also works natively in all modern browsers). No build tools, no package managers, no package.json.

esm.sh processes NPM packages and uses ESBuild under the hood for transpilation. So you actually ARE using a build tool, a package manager and a package.json file. Albeit at a distance.

That said; problem is where? If you're using esm.sh to serve out modules - then you'll have native BigInt support, which would be the default configuration for the package. Which wouldn't need any overrides...

@lionel-rowe
Copy link

That said; problem is where? If you're using esm.sh to serve out modules - then you'll have native BigInt support, which would be the default configuration for the package. Which wouldn't need any overrides...

I'm no longer using this polyfill, I switched to fullcalendar/temporal-polyfill. But the esm.sh build definitely includes jsbi:

/* esm.sh - @js-temporal/polyfill@0.4.4 */
import "/v135/jsbi@4.3.0/es2022/jsbi.mjs";
export * from "/v135/@js-temporal/polyfill@0.4.4/es2022/polyfill.mjs";

@rjgotten
Copy link

rjgotten commented May 7, 2024

That said; problem is where? If you're using esm.sh to serve out modules - then you'll have native BigInt support, which would be the default configuration for the package. Which wouldn't need any overrides...

I'm no longer using this polyfill, I switched to fullcalendar/temporal-polyfill. But the esm.sh build definitely includes jsbi:

/* esm.sh - @js-temporal/polyfill@0.4.4 */
import "/v135/jsbi@4.3.0/es2022/jsbi.mjs";
export * from "/v135/@js-temporal/polyfill@0.4.4/es2022/polyfill.mjs";

Afaict the idea proposed evolved to a point that there was some kind of consensus that the default configuration for the package should be to no longer use JSBI, and then require users that need the JSBI support to somehow override that and have the JSBI-version be served.

In other words- for your particular use case, that idea would work without issue. 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests