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

chore: Everybody gets a package #2218

Merged
merged 27 commits into from
Jan 15, 2020
Merged

chore: Everybody gets a package #2218

merged 27 commits into from
Jan 15, 2020

Conversation

ecraig12345
Copy link
Member

@ecraig12345 ecraig12345 commented Jan 9, 2020 •
edited
Loading

Description

Promote build, docs, e2e, and perf to full packages (for now that just means they have their own package.json) and start moving dependencies from the root to appropriate places. To reduce noice, I left the folders in the same locations--that can be fixed when we do the repo merge.

I also repurposed the @fluentui/internal-tooling package name: removed packages/internal-tooling and put its contents under build, and used the name @fluentui/internal-tooling for the new package in the build folder. That seemed like it would simplify things slightly since everything already has @fluentui/internal-tooling as a dev dependency, but I'm open to other approaches.

Dev workflow impact

New dependencies which are specific to build, docs, e2e, or perf should be added into the package.json for that folder, not the root package.json. (Adding to the root will still work, but is not correct.)

Smaller changes

  • Move config.ts from the root into build -- that's where it's used mostly, and it lets the couple other packages that need it require it from the internal-tooling package
  • Replace gulp-load-plugins with explicit imports from gulp-util -- this is the only plugin which appears to be used currently, and directly importing it makes the dependency more explicit
  • I checked all the places that use lerna-aliases to make sure the new packages won't cause problems, and removed them from the lists in the couple problematic places (they should be harmless in things like moduleNameMapper or resolve.alias)
  • Update most relative imports into build to use @fluentui/internal-tooling instead. Exception is config inheritance, where package-based imports either outright don't work or cause strange issues (see comment about tsconfig).
  • Make @fluentui/internal-tooling not be published going forward

@JasonGore
Copy link
Member

image

layershifter
layershifter reviewed Jan 9, 2020
View reviewed changes
Copy link
Member

@layershifter layershifter left a comment

Choose a reason for hiding this comment

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

Love it 😍

@layershifter
Copy link
Member

Can we also run yarn-deduplicate to remove copies?

https://github.com/atlassian/yarn-deduplicate
It will be also great to integrate it into CI process

@ecraig12345
Copy link
Member Author

yarn-deduplicate looks useful! I'll run it locally and make a note about investigating adding it to CI later.

@ecraig12345
Copy link
Member Author

@layershifter I noticed the internal-tooling package is currently published to npm. Is there a reason for that?

@layershifter
Copy link
Member

@ecraig12345 I don't think so, let's make it private

@ecraig12345 ecraig12345 force-pushed the chore/all-packages branch 2 times, most recently from 524d7e1 to b441d60 Compare January 11, 2020 01:34
@ecraig12345 ecraig12345 changed the base branch from tooling/split-jest to master January 11, 2020 01:47
@ecraig12345 ecraig12345 reopened this Jan 14, 2020
@ecraig12345 ecraig12345 mentioned this pull request Jan 14, 2020
Closed
@layershifter
Copy link
Member

Re: the non-approved dependencies warning, based on yarn.lock it doesn't look like the actual installed version of scheduler has changed. @layershifter or anyone else, do you know what to do about the warning?

I fixed it in a8137b6

@ecraig12345
Copy link
Member Author

@layershifter Thanks! Yeah accidentally updating React is not good!

@ecraig12345
Copy link
Member Author

@layershifter Screener is reporting 1 visual regression, but I don't have permission to view the link. https://screener.io/v2/dashboard/5da858d6bca5864d660633ed/2218

@ecraig12345
Copy link
Member Author

Important learning, DO NOT use package names for local packages in tsconfig extends!! Otherwise it will resolve paths, such as types, relative to the file's symlink location in node_modules (not its realpath) which results in incorrect type resolution. I changed all the tsconfig extends back to relative paths to fix.

To clarify this: the issue is that it's misinterpreting the typeRoots as being relative to the symlink location. I looked at the configs in the staging repo for comparison--they use a package name in extends, but the shared config doesn't have typeRoots.

This isn't an immediate problem, but if we did want to get rid of the relative paths, some possible solutions:

  • Only packages which actually need custom types define typeRoots in their own package.json
  • Make each custom typings module into an unpublished "package" within the monorepo, named @types/whatever, so it gets linked into the proper location and we don't have to modify typeRoots
  • I tried making the shared config into a JS file so it can programmatically set a proper baseUrl when imported, but TS doesn't support that

@ecraig12345 ecraig12345 mentioned this pull request Jan 15, 2020
Open
@ecraig12345 ecraig12345 merged commit 0247c33 into master Jan 15, 2020
@ecraig12345 ecraig12345 deleted the chore/all-packages branch January 15, 2020 23:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kenotron kenotron kenotron approved these changes

@layershifter layershifter layershifter approved these changes

@dvdzkwsk dvdzkwsk Awaiting requested review from dvdzkwsk

@dzearing dzearing Awaiting requested review from dzearing

@JasonGore JasonGore Awaiting requested review from JasonGore JasonGore is a code owner

@jdhuntington jdhuntington Awaiting requested review from jdhuntington

@levithomason levithomason Awaiting requested review from levithomason levithomason is a code owner

@miroslavstastny miroslavstastny Awaiting requested review from miroslavstastny miroslavstastny is a code owner

@mnajdova mnajdova Awaiting requested review from mnajdova

Assignees
No one assigned
Labels
🗑️ chore 🛠️ tooling All things tooling
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ecraig12345 @JasonGore @layershifter @DustyTheBot @kenotron