Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

chore: Set packages' "main" to src/index locally to help type resolution #2253

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ecraig12345
Copy link
Member

These changes to resolution of internal packages will hopefully make removing aliases in #2243 (and various other work) go more smoothly and prevent the relative paths in inferred types issue.

Remove tsconfig.json paths where possible, since that seems to cause the relative import issue. Instead, in package.json set "main": "src/index.ts" (and add a pre-publish task to switch it back). This is what @kenotron did in staging and it lets imports from the package root work without a paths config.

I also removed the the types, module, and jsnext:main settings from most package.jsons, and updated the pre-publish task to add them back. Having types defined as a generated file (even with main pointing to src) definitely seemed to prevent references from working properly, but I thought I'd also remove the other two for consistency?

Other related changes:

  • Remove packages/react from includes of other projects and use dependencies instead.
  • Realized I forgot to add all the packages as actual deps of the docs project
  • Add a couple missing project references and composite: true settings
  • Delete some unused config files

// { "path": "../packages/react" },
// { "path": "../packages/react-component-event-listener" },
// { "path": "../packages/react-component-ref" },
// { "path": "../packages/styles" }
Copy link
Member

Choose a reason for hiding this comment

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

Comments there are cruft?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, I'll remove or uncomment this before checkin. I was curious if docs could build with project references but I'm not done testing yet.

+ "jsnext:main": "dist/es/index.js",
+ "main": "dist/commonjs/index.js",
+ "module": "dist/es/index.js",
+ "types": "dist/es/index.d.ts",
Copy link
Member

Choose a reason for hiding this comment

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

These entries are cruft?

scripts/tasks/publishPrepareTask.ts Show resolved Hide resolved
@layershifter
Copy link
Member

Overall looks good 👍 But please ensure that publishing will not be broken 🙏

@ecraig12345
Copy link
Member Author

From doing some experimenting I learned that with project references, if you make package.json main and typings plus tsconfig.json outDir point to the same directory, build-less intellisense (and build mode compiling) will "just work" with NO additional tsconfig paths settings and without the main: src/index.ts hack. Example here.

However, when I tried setting it up in this repo (with updated pre-publish logic to correct main and types), I got stuck on webpack because ForkTSCheckerWebpackPlugin does not yet support project references. (Neither does ts-loader, which is more relevant for @JasonGore's work on Fabric.) You can see what I tried in the chore/full-project-refs branch.

So now I'm back to trying to get everything working with the main: src/index.ts hack.

Also noticed that some tools don't seem to respect the types field, only typings. So it's possible we should consider switching to that.

cc @layershifter @kenotron

@ecraig12345 ecraig12345 changed the base branch from master to chore/custom-types January 29, 2020 00:52
@ecraig12345 ecraig12345 changed the base branch from chore/custom-types to master January 29, 2020 21:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants