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

chore: Remove src, test, build, docs path aliases #2233

Closed
wants to merge 14 commits into from

Conversation

ecraig12345
Copy link
Member

@ecraig12345 ecraig12345 commented Jan 14, 2020

Replace legacy path aliases with more standard imports:

  • Replace src with @fluentui/react everywhere.
    • For deep imports, I used paths under @fluentui/react/src. I don't think this should create problems since those imports are only used in tests and docs, but please let me know if that's wrong.
  • Replace test with relative paths
  • Replace build with relative paths (a couple to be changed to @fluentui/internal-tooling once chore: Everybody gets a package #2218 is done)
  • Replace docs in docs project with relative paths
  • A few doc-related files in build/gulp/plugins/util import from docs/src/types. For now, add a file in that directory which re-exports the types (imported by relative path) and update all imports to be from that file. Eventually the types should move to a shared location.
  • Replace docs imports in perf and perf-test with relative path imports, to be removed once the @fluentui/docs package is added in chore: Everybody gets a package #2218
  • A couple tests in react and accessibility import generated files which currently live under docs (componentInfo, behaviorInfo). These imports have to stay as path imports for now because having those packages depend on @fluentui/docs would cause circular imports. So we need a better all-up solution for where these generated files and/or the tests that rely on them should live (or get rid of the generated files?).

@layershifter
Copy link
Member

tenor

import { mountWithProviderAndGetComponent, mountWithProvider } from 'test/utils'
import { UIComponent } from 'src/utils'
import { mountWithProviderAndGetComponent, mountWithProvider } from '../../utils'
import { UIComponent } from '@fluentui/react/src/utils'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { UIComponent } from '@fluentui/react/src/utils'
import { UIComponent } from '../../src/utils'

Can we use instead for unexported things?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's kind of strange to have a combination of relative path imports and package name imports into the same code, and it might not be clear to a developer coming into the code which import type to use when. (Though you could make the same argument about clarity with when to use @fluentui/react vs paths like @fluentui/react/src/whatever.)

import { mountWithProvider } from 'test/utils'
import { Props, PropsOf, InstanceOf } from 'src/types'
import { mountWithProvider } from '../../utils'
import { Props, PropsOf, InstanceOf } from '@fluentui/react/src/types'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { Props, PropsOf, InstanceOf } from '@fluentui/react/src/types'
import { Props, PropsOf, InstanceOf } from '../../src/types'

Same thing...

@ecraig12345
Copy link
Member Author

ecraig12345 commented Jan 14, 2020

Build failure (at least one of them) is because somehow the generated .d.ts is now using relative paths for inferred types?? This is CarouselNavigation.d.ts before and after:
image

@@ -3,10 +3,7 @@
"compilerOptions": {
"module": "esnext",
"paths": {
Copy link
Member

Choose a reason for hiding this comment

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

paths can be removed there

Copy link
Member

Choose a reason for hiding this comment

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

Can you please also check include. I don't think that we should include react anymore

@layershifter
Copy link
Member

Build failure (at least one of them) is because somehow the generated .d.ts is now using relative paths for inferred types?? This is CarouselNavigation.d.ts before and after:
image

Looks like. And we don't have any ideas why it happens, it's insane 💣 Can you please try tomerge latest master it should not occur there.

@ecraig12345 ecraig12345 force-pushed the chore/remove-src-paths-2 branch from 0ff4a1d to e17385f Compare January 14, 2020 22:47
@ecraig12345
Copy link
Member Author

Looks like. And we don't have any ideas why it happens, it's insane 💣 Can you please try tomerge latest master it should not occur there.

I think it was somehow introduced by this set of changes...not sure how. I'm going to work on getting an isolated repro.

@ecraig12345
Copy link
Member Author

Closing this down and splitting the changes.

@ecraig12345 ecraig12345 deleted the chore/remove-src-paths-2 branch January 15, 2020 21:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants