GoogleChromeLabs / jsbi

JSBI is a pure-JavaScript implementation of the official ECMAScript BigInt proposal.
https://v8.dev/features/bigint#polyfilling-transpiling
Apache License 2.0
919 stars 68 forks source link

UMD bundle of v3.2.5 will throw TypeError: h.__initializeDigits is not a function #79

Closed Leslie-Wong-H closed 3 years ago

Leslie-Wong-H commented 3 years ago

Usage:

image

The Webpack build bundle will throw TypeError:

image

Work-around:

Downgrade to v3.1.4 could prevent the TypeError.

12wrigja commented 3 years ago

Can you provide a minimal repro for this error? What operation were you attempting to perform?

Leslie-Wong-H commented 3 years ago

Literally, I am utilizing JSBI to implement BigDecimal for arbitrary arithmetic expression computation purposes, within a Vue2.6+Webpack3+Babel6 project. When in v3.1.4, if I write:

import JSBI from 'jsbi'

It refers to the cjs file, and the result is:

"export 'default' (imported as 'JSBI') was not found in 'jsbi'
Cannot read properties of undefined (reading 'BigInt')

The same as

import JSBI from 'jsbi/dist/jsbi-umd'

It works when

import JSBI from 'jsbi/dist/jsbi.mjs'

However, the mjs file contains too many static class properties, which the project doesn't resolve well (need to be compatible with ie11), and my colleague suggests me the final solution, namely

const JSBIUMD = require('jsbi/dist/jsbi-umd')
const JSBI = JSBIUMD.JSBI

along with the webpack include statement to solve the 'let' declaration in jsbi-umd.

include: [
    resolve('/node_modules/jsbi/dist/jsbi-umd.js'),
     ...
]

It functions seamlessly, yet ironically I am using ESM at the end of the file.

export { bigDecimalResult, jsbiCal, rpnParse }

It seems Webpack does handle the mix-module statements well.

Finally, comes the TypeError. It is ^3.1.4 at package.json, hence my colleague fetched the v3.2.5 package of jsbi. And the TypeError above appeared.

Are there any UMD bundle target differences between the previous build config and the current ts-refactored one?

12wrigja commented 3 years ago

There was #70 which was concerned with the default export not working, but that was fixed in v3.2.2.

My previous experience with webpack was that it would prefer to load the UMD bundles over ESM or CJS bundles, which was odd. I'm surprised that you say it was loading the cjs bundle - I'd have expected it to use the UMD bundle.

However, the mjs file contains too many static class properties, which the project doesn't resolve well (need to be compatible with ie11)

Isn't this something that something like Babel can take care of for you? What problems specifically with IE11 did you run into?

Can you create a minimal repro project that uses your build configuration for us to inspect?

Leslie-Wong-H commented 3 years ago

After snooping through the umd bundle codes of v3.1.4 and v3.2.5, I figure out the reason of this issue.

The place where __initializeDigits is defined in the umd bundle codes of v3.1.4. https://unpkg.com/jsbi@3.1.4/dist/jsbi-umd.js image

However, within the bundle of v3.2.5, __initializeDigits is defined as a non-static method of class o, namely class JSBI. https://unpkg.com/jsbi@3.2.5/dist/jsbi-umd.js image

And the position of the error code (Sorry it also appears when yarn dev, thankfully easy to debug) image

Then I locate it at the bundle. image

console.log u from the project. image

And the result. Oops! The prototype of it is Array, not a JSBI-Defined Object, hence method __initializeDigits is lost. image

As a comparison, here is how it should normally works as a vanilla html file opened via firefox.

image

I have no idea why webpack or babel is not able to profile JSBI class prototype correctly, for the moment, because class can not be hoisted or something? As for the build config change of JSBI, I think this may be the root reason.

// tsconfig.json
    "target": "es2015",

Finally, I suppose this commit is no longer working. image

BTW, thanks for correcting my mindset about the precedence of main, module and browser fields of package.json.

12wrigja commented 3 years ago

Sorry I didn't see this till now - I think I'm running into similar issues in a different environment and was reminded of this issue.

Did you try rebuilding using a higher target in the tsconfig (maybe even just ESNext?) and seeing what webpack produces as output? I don't think the typescript target level matters a whole lot, and if we can fix the UMD bundle as a result of changing it that sounds good.

Finally, I suppose this commit is no longer working.

Can you elaborate?

Leslie-Wong-H commented 3 years ago

Since the Webpack version is kind of outdated for the project I mentioned, I don't think that is the focus of this issue, after the above investigation.

Instead, we should manage to keep the bundles of this project as consistent as they were like in 3.1.*, to prevent disaster for the downstream consumers. The main goal of TS migration should be to catch more implicit bugs within the project.

After carefully inspecting the TS migration PR #67, I spot some contradictions.

  1. The babel() statement at rollup.config.js makes no effect on the generated UMD bundle.
    {
    input: input,
    plugins: [
      typescript(),
      babel(),

    At the first attempt, I directly ran "npm run build", formatting the dist code, then commented "babel()", "npm run build" and formation again. Finally, use the "compare two files" functionality of VScode to compare the UMD bundles. Surprisingly, they are identical. See results comparing es2015_babel/jsbi-umd.js and es2015_nobabel/jsbi-umd.js

Next, I changed the target at tsconfig.json to "es5" and repeated the above steps. Once again they are identical. See es5_babel and es5_nobabel

  1. The npm script "build" is antilogous.

    "build": "tsc && for f in tsc-out/*.js; do mv -- \"$f\" \"${f%.js}.mjs\"; done && rollup --config rollup.config.js",

The latter command "rollup --config rollup.config.js" makes tsc-out/jsbi.mjs utterly nonsense in the final bundles, given that the "input" variable on rollup.config.js is "lib/jsbi.ts", not "tsc-out/jsbi.mjs". So the latter command just ignores the former tsc, and then "typescript()" on rollup.config.js will do the tsc compilation process again. Duplicate compilation processes. At this point. "typescript()" will read tsconfig.json, set the target to es2015, and then bundle, unified for cjs, umd and mjs. (babel not working concerning the above observation.) However, if we look at the bundle code of v3.1.6, cjs and mjs are es2015-like (containing "static"), and umd is es5 compatible (no "static"). The formatted 3.1.6 bundle codes are here.

image

image

image

  1. The bundle process needs to change despite the rhetoric that TypeScript can not handle class inheritance in es5 well.
    // tsconfig.json
    target: "es2015"

    The target is reasonable concerning the rhetoric, but it should not account for generating cjs, mjs and umd bundles unifiedly, ruining the previous endeavor to make JSBI browser-friendly. After all, the existence of JSBI is to provide BigInt support for older browsers. Yet now it is gone. To fix this, clearly a better and safe bundle process is to set tsc-out/jsbi.mjs as input on rollup.config.js, and remove the rollup-typescript package, which also makes "build" more accountable. In this way, the introduction of TypeScript is only to enhance dev experience (catch bug earlier at compile time), and leave the bundle grind to babel, just as before, resolving the problem of compiling class inheritance in es5 well.

The bundle output of my attempt by commenting "typescript()" and set "const input = 'tsc-out/jsbi.mjs';". Link

image

image

image

Leslie-Wong-H commented 3 years ago

Related discussion: The npm script "dev" should change to "tsc --watch", after the bundle process reform above. The "dev" is not related to the bundle. Keep bundle separated and consistent?

12wrigja commented 3 years ago

I've opened a PR to fix the issues with the prototype, but this doesn't resolve the issues with the build system not actually optimizing this for IE11 (e.g ES5 output).

12wrigja commented 3 years ago

@Leslie-Wong-H Your proposal above to split out using the typescript plugin and instead use the .mjs output directly sounds good to me. Would you like to send a PR to implement that? If not I can send a PR sometime this week. Thanks for digging into this.

My guess as to why Babel isn't transpiling properly is because it of the file extensions involved in the output from the typescript plugin - see https://github.com/rollup/rollup-plugin-babel/issues/255.