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

add script to copy types for commonjs module system #184

Merged
merged 1 commit into from
Oct 12, 2022

Conversation

shrujalshah28
Copy link
Contributor

Fix: #177

@shrujalshah28
Copy link
Contributor Author

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

@12wrigja 12wrigja self-requested a review October 7, 2022 17:55
@12wrigja
Copy link
Contributor

12wrigja commented Oct 7, 2022

@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
Copy link
Contributor

12wrigja commented Oct 7, 2022

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
Copy link
Contributor Author

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

@12wrigja
Copy link
Contributor

12wrigja commented Oct 7, 2022

Thanks. I'd be especially interested to see if it's different from the repro in #177 (comment) - I think this is the same problem / fix, but want to be sure.

@shrujalshah28
Copy link
Contributor Author

@12wrigja Here is repo.

@12wrigja
Copy link
Contributor

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
Copy link
Contributor Author

shrujalshah28 commented Oct 11, 2022

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
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Contributor

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!

@12wrigja 12wrigja merged commit 9bab0eb into js-temporal:main Oct 12, 2022
@shrujalshah28 shrujalshah28 deleted the patch-2 branch October 12, 2022 17:39
@12wrigja 12wrigja mentioned this pull request Oct 12, 2022
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

Successfully merging this pull request may close these issues.

Types not found when moduleResolution: node16 is set for typescript
2 participants