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

Delete test-gc-sweep-tests #15943

Merged
merged 2 commits into from
Jun 12, 2023
Merged

Delete test-gc-sweep-tests #15943

merged 2 commits into from
Jun 12, 2023

Conversation

Abe27342
Copy link
Contributor

@Abe27342 Abe27342 commented Jun 9, 2023

Description

This makes progress toward #15917 by deleting test-gc-sweep-tests, which is not needed anymore as per @agarwal-navin.

PACKAGES.md was updated using flub check layers --info ./build-tools/packages/build-tools/data/layerInfo.json --md ..

@Abe27342 Abe27342 requested review from msfluid-bot and a team as code owners June 9, 2023 22:31
@github-actions github-actions bot added area: tests Tests to add, test infrastructure improvements, etc base: main PRs targeted against main branch labels Jun 9, 2023
@msfluid-bot
Copy link
Collaborator

msfluid-bot commented Jun 9, 2023

@fluid-example/bundle-size-tests: +30 Bytes
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 448.83 KB 448.84 KB +6 Bytes
connectionState.js 680 Bytes 680 Bytes No change
containerRuntime.js 239.47 KB 239.47 KB +2 Bytes
loader.js 154.4 KB 154.4 KB +4 Bytes
map.js 46.66 KB 46.66 KB +2 Bytes
matrix.js 146.59 KB 146.59 KB +2 Bytes
odspDriver.js 92.31 KB 92.32 KB +6 Bytes
odspPrefetchSnapshot.js 43.64 KB 43.64 KB +4 Bytes
sharedString.js 163.26 KB 163.27 KB +2 Bytes
sharedTree2.js 279.92 KB 279.92 KB No change
Total Size 1.73 MB 1.73 MB +30 Bytes

Baseline commit: 56bd728

Generated by 🚫 dangerJS against f8122bf

import { ContainerManager } from "../containerManager";
import { rootDataObjectWithChildDataObjectFactory } from "../dataObjectWithChildDataObject";
import { dataObjectWithCounterFactory } from "../dataObjectWithCounter";
import { mockConfigProvider } from "../mockConfigProvider.js";
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering if this would lose typing info for intellisense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope. Typescript looks for .d.ts files adjacent to the js file when using an import statement from an esnext module. You can see their doc has similar syntax.

Overall, my impression is that the ecosystem is a lot more mature w.r.t. supporting fully qualified import paths than the last time I checked. E.g. the changes I had to make to webflow's webpack config to make it understand this import were pretty trivial:

				// This ensures that webpack understands fully-specified relative module imports.
				// See https://github.com/webpack/webpack/issues/13252 for more discussion.
				extensionAlias: {
					".js": [".ts", ".tsx", ".js"],
					".mjs": [".mts", ".mtsx", ".mjs"],
				},

The 2 biggest issues I encountered which I don't have ideal solutions for were:

  1. VSCode will generate incorrect autocomplete imports. This behavior is configurable via "typescript.preferences.importModuleSpecifierEnding", but I don't think there's a way to tell VSCode to scope the setting to particular subsets of our repo. This issue would go away if we ever convert to full ESNext modules only (which is probably a ways off).
  2. I turned off ESLint's "import/no-unresolved" rule for relative imports. We could fix this if we wrote our own ESLint import resolver, but I didn't find one that worked out of the box and IMO the maintenance burden is probably not worth the value the lint rule provides for relative imports.

@Abe27342 Abe27342 requested a review from a team as a code owner June 12, 2023 17:42
@Abe27342 Abe27342 changed the title Covert test-gc-sweep-tests to use esnext modules Delete test-gc-sweep-tests Jun 12, 2023
@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label Jun 12, 2023
@Abe27342 Abe27342 merged commit 04fbcb5 into microsoft:main Jun 12, 2023
@Abe27342 Abe27342 deleted the gc-tests-esm branch June 12, 2023 18:22
connorskees pushed a commit to connorskees/FluidFramework that referenced this pull request Jul 19, 2023
## Description

This makes progress toward microsoft#15917 by deleting test-gc-sweep-tests, which
is not needed anymore as per @agarwal-navin.

PACKAGES.md was updated using `flub check layers --info
./build-tools/packages/build-tools/data/layerInfo.json --md .`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: tests Tests to add, test infrastructure improvements, etc base: main PRs targeted against main branch dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants