mentat-collective / emmy

The Emmy Computer Algebra System.
https://emmy.mentat.org
GNU General Public License v3.0
405 stars 24 forks source link

fix(extend-type): use bigint instead of BigInt #157

Closed mhuebert closed 7 months ago

mhuebert commented 9 months ago

I was seeing this warning on recent cljs:

WARNING: Extending an existing JavaScript type - use a different symbol name instead of js/BigInt

Using bigint instead of js/BigInt when extending types/protocols eliminates this warning. via @shaunlebron:

The "function", "object", "array" symbols are just symbols that have meaning in the context of the extend-type macro, so nothing special syntactically. They just map internally to the type strings produced by goog/typeOf, used for protocol dispatch.

I verified that (goog/typeOf (js/BigInt 1)) => "bigint"

Discussion: https://groups.google.com/g/clojurescript/c/MKEZ9CBU77o?pli=1

codecov[bot] commented 9 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (13dda79) 87.78% compared to head (1919ddc) 87.77%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #157 +/- ## ========================================== - Coverage 87.78% 87.77% -0.02% ========================================== Files 100 100 Lines 15965 15965 Branches 852 853 +1 ========================================== - Hits 14015 14013 -2 - Misses 1098 1099 +1 - Partials 852 853 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

sritchie commented 9 months ago

@mhuebert thank you for this! The linter doesn't like it but I bet that's a bug we can... bug... @borkdude about. @mhuebert do you know if this still works in older versions of cljs?

borkdude commented 9 months ago

This is new CLJS 1.11.132 behavior right? For now configure clj-kondo with {:linters {:unresolved-symbol [bigint]}} I'll fix it in the next release of clj-kondo (unfortunately just had one today already)

sritchie commented 9 months ago

Thanks @borkdude !

sritchie commented 7 months ago

Interesting, @mhuebert this seems to fail with the version of cljs used in the tests;

TypeError: Cannot read properties of undefined (reading 'prototype')
    at /home/runner/work/emmy/emmy/.shadow-cljs/builds/test/dev/out/cljs-runtime/emmy/value.cljc:245:4
    at global.SHADOW_IMPORT (/home/runner/work/emmy/emmy/target/main/node-tests.js:64:44)
    at /home/runner/work/emmy/emmy/target/main/node-tests.js:1582:1
    at Object.<anonymous> (/home/runner/work/emmy/emmy/target/main/node-tests.js:1933:3)
    at Module._compile (node:internal/modules/cjs/loader:1198:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1252:10)
    at Module.load (node:internal/modules/cjs/loader:1076:32)
    at Function.Module._load (node:internal/modules/cjs/loader:911:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
    at node:internal/main/run_main_module:22:47
Error: Process completed with exit code 1.

I wonder if this is not backwards compatible. I'm confused since based on that group post it should be...

borkdude commented 7 months ago

I don't think it's backwards compatible, since the support for this was explicitly added in a new version of CLJS. There is no way to make prior versions of CLJS behave differently?

sritchie commented 7 months ago

For sure, I just didn't know if the warning was new or the functionality for bigint.

sritchie commented 7 months ago

@mhuebert , I'm going to close this so that we keep backwards compatibility - lmk and we can re-open if that's a problem. Thank you for finding this, let's do it when appropriate!