js-temporal / temporal-polyfill

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

add script to copy types for commonjs module system #184

Closed shrujalshah28 closed 2 years ago

shrujalshah28 commented 2 years ago

Fix: #177

shrujalshah28 commented 2 years ago

I just tested the script, but Haven't tried to publish it.

12wrigja commented 2 years ago

@shrujalshah28 Let's focus the review here, and drop https://github.com/js-temporal/temporal-polyfill/pull/181/files as is duplicated with this.

12wrigja commented 2 years ago

Based on the way other projects are fixing their types for Node's ESM support, I think this is the right fix. Once the key is included for the default this should be good to merge.

@shrujalshah28 if you have a repo setup to use Node's ESM support that I can use to verify this fixes things, that would be most helpful.

shrujalshah28 commented 2 years ago

@12wrigja Currently I use ESM in private repository but tomorrow I can create small repository to verify changes.

12wrigja commented 2 years ago

Thanks. I'd be especially interested to see if it's different from the repro in https://github.com/js-temporal/temporal-polyfill/issues/177#issue-1325208301 - I think this is the same problem / fix, but want to be sure.

shrujalshah28 commented 2 years ago

@12wrigja Here is repo.

12wrigja commented 2 years ago

With your repro repo, I think we would also need fixes to JSBI (our only dependency) in order for this to work - without also changing the JSBI package.json, the temporal project doesn't seem to compile anymore:

> @js-temporal/polyfill@0.4.2 build
> rm -rf dist/* tsc-out/* && tsc && rollup -c rollup.config.js

lib/duration.ts:21:18 - error TS2307: Cannot find module 'jsbi' or its corresponding type declarations. 

21 import JSBI from 'jsbi';                                                                            
                    ~~~~~~

lib/ecmascript.ts:23:18 - error TS2307: Cannot find module 'jsbi' or its corresponding type declarations.

23 import JSBI from 'jsbi';                                                                            
                    ~~~~~~                         

lib/instant.ts:9:18 - error TS2307: Cannot find module 'jsbi' or its corresponding type declarations.

9 import JSBI from 'jsbi';

JSBI's package.json is set up a bit differently than ours and doesn't have exports at all. I'm wondering whether introducing exports could be a breaking change.

shrujalshah28 commented 2 years ago

Ohh, I am not an expert on this but I will try this and find the solution.

I'm wondering whether introducing exports could be a breaking change.

Yes

12wrigja commented 2 years ago

I'm also an owner for JSBI, so I'll chat with the other maintainers over the next couple days and see if we can fix JSBI up as well. It's very unfortunate that this has a transitive effect on the ecosystem.

shrujalshah28 commented 2 years ago

@12wrigja When I try to build in my branch, It works fine. I am able to build packages.

some\path\temporal-polyfill>npm run build

> @js-temporal/polyfill@0.4.2 build
> tsc && rollup -c rollup.config.js

tsc-out/index.js → ./dist/index.esm.js, ./dist/index.cjs...
created ./dist/index.esm.js, ./dist/index.cjs in 2.1s

tsc-out/index.js → ./dist/index.umd.js...
Browserslist: caniuse-lite is outdated. Please run:
  npx browserslist@latest --update-db
  Why you should do it regularly: https://github.com/browserslist/browserslist#browsers-data-updating
created ./dist/index.umd.js in 7.1s
12wrigja commented 2 years ago

I couldn't find a way to repro the error I saw above in local testing when patching your repro and this branch, so I think we are good to merge this.

Thanks for your help figuring this out!