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

Respect custom tsconfig path for esModuleInterop #436

Merged
merged 1 commit into from
Feb 26, 2020
Merged

Respect custom tsconfig path for esModuleInterop #436

merged 1 commit into from
Feb 26, 2020

Conversation

tricoder42
Copy link
Contributor

When custom tsconfig is used (e.g. tsdx build --tsconfig tsconfig.build.json), it is ignored in createRollupConfig.

For example, this causes bug in reach-ui build reach/reach-ui#434. They use tsconfig.build.json, which is being ignore and the build files are missing Object.defineProperty(exports, '__esModule', { value: true });.

Copy link
Collaborator

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this @tricoder42 !! Thought I might've missed something when I fixed the esModuleInterop setting (#327 ). Would be good to have a test for custom tsconfig paths to check for these types of bugs.

Could you rename the PR & commit to "fix: respect custom tsconfig path for esModuleInterop" to be more specific?
It's a bit misleading in its current form, as custom tsconfigs are used internally and custom tsconfig path is used internally by rollup-plugin-typescript2 (see this line), but not for the esModuleInterop setting (which is a more recent feature/fix)

@tricoder42 tricoder42 changed the title Rollup config doesn't respect custom tsconfig Respect custom tsconfig path for esModuleInterop Jan 21, 2020
@tricoder42
Copy link
Contributor Author

@agilgur5 Both commit and PR were renamed. Thanks for the review! 👍

@chaance
Copy link

chaance commented Jan 24, 2020

Hey @sw-yx / @agilgur5 anything I can do to help get this merged, or perhaps maybe you know of a workaround in the mean time? A few of our packages in Reach UI use default exports and are broken as it stands. Many thanks for your work on this!

@agilgur5
Copy link
Collaborator

agilgur5 commented Jan 24, 2020

@chancestrickland I'm an active contributor but don't have merge privileges -- would merge & release a patch if I could! If @sw-yx didn't merge I assume he's waiting for Jared's approval? Jared also handles releases.

I think you should be able to workaround it by overriding the rollup config with tsdx.config.js. Would give an example myself but don't have time right now (traveling till next week), there are some in #379 and other issues.

@chaance
Copy link

chaance commented Jan 24, 2020

@agilgur5 Excellent, thanks for the link and super-quick reply. I'll see if I can go that route for now. 👍

@swyxio
Copy link
Collaborator

swyxio commented Jan 24, 2020

yea usually for anything config-centric i want to make sure jared approves everything personally since thats the core of tsdx. i dont want to make decisions that he then has to reverse. he also handles the deploy. so leaving to @jaredpalmer.

dont forget you can also do local forks while u wait for pr's to get merged, with http://npm.im/patch-package

@chaance
Copy link

chaance commented Jan 24, 2020

I genuinely did not know about patch-package. This may be the best day of my life.

@agilgur5
Copy link
Collaborator

yea usually for anything config-centric i want to make sure jared approves everything personally since thats the core of tsdx. i dont want to make decisions that he then has to reverse.

For sure. I thought one-liner bugfixes like this might fall into a different category since they're not really changing any behavior tho (I feel like I've made a few PRs like that but idr)

@jaredpalmer
Copy link
Owner

Sorry for delay. Will get this out now

@jaredpalmer jaredpalmer merged commit 4ec56fc into jaredpalmer:master Feb 26, 2020
@agilgur5
Copy link
Collaborator

@all-contributors please add @tricoder42 for bug, code

@allcontributors
Copy link
Contributor

@agilgur5

I've put up a pull request to add @tricoder42! 🎉

@agilgur5
Copy link
Collaborator

agilgur5 commented Mar 7, 2020

Hmmm, so after writing #526 , I wonder if this will work properly without using resolveApp. Will need to add more tests around this.

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.

5 participants