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

[TypeScript] FileSystem migration #3340

Merged
merged 10 commits into from
Jan 31, 2019

Conversation

EvanBacon
Copy link
Contributor

How

Changed default import

To optimize our libraries for dead code elimination we should migrate our exports to be imported as such:

- import { FileSystem } from 'expo-file-system';

+ import * as FileSystem from 'expo-file-system';

Ideally we would make the main entry-point of a module be a file named like the module:
src/FileSystem.ts (build/FileSystem.js).
To migrate I propose we do what expo-constants does:

src/index.ts

import * as FileSystem from './FileSystem';
export * from './FileSystem';

let wasImportWarningShown = false;
// @ts-ignore: Temporarily define an export named "FileSystem" for legacy compatibility
Object.defineProperty(exports, 'FileSystem', {
  get() {
    if (!wasImportWarningShown) {
      console.warn(
        `The syntax "import { FileSystem } from 'expo-file-system'" is deprecated. Use "import * as FileSystem from 'expo-file-system'" or import named exports instead. Support for the old syntax will be removed in SDK 34.`
      );
      wasImportWarningShown = true;
    }
    return FileSystem;
  },
});

Then eventually remove the index in favor of the named file. (src/FileSystem.ts)

Added tests

Migration should include the addition of a src/__tests__ which can be run with yarn test in the root directory of the package.

If the package is using the old structure of test/ for utilities, they should migrate to using jest-expo.

- import { mockPlatformWeb } from '../../test/mocking';

+ import { mockPlatformWeb } from 'jest-expo';

Moved native dependencies to peerDependencies

In order to prevent overlapping native code in node_modules, we should move any dependencies containing native code to peerDependencies.

Add tsconfig.json

/tsconfig.json

// @generated by expo-module-scripts
{
  "extends": "expo-module-scripts/tsconfig.base",
  "compilerOptions": {
    "outDir": "./build"
  },
  "include": ["./src"],
  "exclude": ["**/__mocks__/*", "**/__tests__/*"]
}

Various other changes:

  • Removed babel-preset-expo
  • Removed flow (obviously)
  • Add scripts:
"scripts": {
    "build": "expo-module build",
    "clean": "expo-module clean",
    "test": "expo-module test",
    "prepare": "expo-module prepare",
    "prepublishOnly": "expo-module prepublishOnly",
    "expo-module": "expo-module"
}

Converting Flow to TypeScript

Proposal

// Flow
type Foo = 'dragon' | 'slayer';

// TS
enum Foo {
  Dragon = 'dragon',
  Slayer = 'slayer',
}

For reusing types across web native-layer and API-layer, types should be moved to a named file with the .types.ts extension.

Related

#2164

@EvanBacon EvanBacon added FileSystem TypeScript Migrating or fixing TypeScript labels Jan 26, 2019
@EvanBacon EvanBacon self-assigned this Jan 26, 2019
@EvanBacon EvanBacon requested a review from ide January 26, 2019 23:27
@ide
Copy link
Member

ide commented Jan 28, 2019

Still need to review the code. These are responses to the PR description:

  • import/export style: yes, using named exports will work better with eliminating unused exports (aka "tree shaking").
  • index.ts file: I think the migration plan is good. We also haven't been doing it in other modules, though, since it's a fairly straightforward breaking change and almost everyone who uses these modules does so through the expo package. So I think it's fine just to make the breaking change.
  • Tests: yes both to adding tests and using jest-expo instead of ../../test/mocking
  • tsconfig.json: let expo-module-scripts generate it. You can make custom changes if absolutely needed (remove the @generated comment or else expo-module-scripts may clobber your changes) but in general we want to centralize our configurations in expo-module-scripts for this repo.
  • Babel preset: yes, remove it. Babel configuration is handled by expo-module-scripts.
  • Removing Flow: yes
  • Adding scripts: looks good
  • Type conversion: type Foo = 'dragon' | 'slayer'; works in TS too in pretty much the same way and keeping the API the same is the least intrusive option. The example code is a breaking change since TS will reject strings passed into places that expect one of the enum values, even string literals with the same values.
  • .types.ts files: seems fine for sharing across platforms. Also in some scenarios it might make sense to break up a .types.ts file into smaller files (e.g. in AV, maybe one for audio and one for video, if that feels right). For types that aren't used across platforms, they can continue to be defined in the files where they're used.

* Updated FileSystem import for `Asset` tests
Copy link
Member

@ide ide left a comment

Choose a reason for hiding this comment

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

New tests look good, added some comments

packages/expo-file-system/src/__tests__/FileSystem-test.ts Outdated Show resolved Hide resolved
packages/expo-file-system/src/__tests__/FileSystem-test.ts Outdated Show resolved Hide resolved
packages/expo-file-system/src/__tests__/FileSystem-test.ts Outdated Show resolved Hide resolved
packages/expo-file-system/src/FileSystem.types.ts Outdated Show resolved Hide resolved
packages/expo-file-system/src/FileSystem.types.ts Outdated Show resolved Hide resolved
packages/expo-file-system/src/FileSystem.types.ts Outdated Show resolved Hide resolved
packages/expo-file-system/src/FileSystem.types.ts Outdated Show resolved Hide resolved
* Refined tests
packages/expo/src/Expo.ts Outdated Show resolved Hide resolved
@EvanBacon EvanBacon merged commit 2307aac into master Jan 31, 2019
@EvanBacon EvanBacon deleted the @evanbacon/typescript/expo-file-system branch January 31, 2019 04:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FileSystem TypeScript Migrating or fixing TypeScript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants