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

Migrate package to TS #68

Merged
merged 2 commits into from
Aug 31, 2022
Merged

Migrate package to TS #68

merged 2 commits into from
Aug 31, 2022

Conversation

aralroca
Copy link
Collaborator

@aralroca aralroca commented Dec 5, 2021

#60

It's working in progress. I have put the following comment // @ts-nocheck, but before merging this PR has to be deleted so that all types are corrected.

  • Fix all TypeScript errors + remove // @ts-nocheck comments
  • Fix eslint to format .ts and .tsx files
  • Move types to another file
  • Reduce to less than 1kb

@aralroca aralroca added this to the Before 1.0.0 milestone Dec 5, 2021
@ruslan-byondxr
Copy link

I think it's a good idea to move type definitions to separate file, then import those types with type-imports.
https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-8.html
https://levelup.gitconnected.com/improving-babel-support-for-typescript-with-type-only-imports-28cb209d9460

@danielart
Copy link
Collaborator

I think it's a good idea to move type definitions to separate file, then import those types with type-imports. https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-8.html https://levelup.gitconnected.com/improving-babel-support-for-typescript-with-type-only-imports-28cb209d9460

Agree, we'll do it before merge. Thanks!

@creotip
Copy link

creotip commented Dec 8, 2021

Cloned this branch. Getting typescript error
Screen Shot 2021-12-08 at 11 10 10

To fix it, you have to install @babel/eslint-parser:
yarn add eslint @babel/core @babel/eslint-parser -D

Then in your .eslintrc.js add parser property:
'parser': '@babel/eslint-parser',

The file should be:

module.exports = {
  'env': {
    'browser': true,
    'es2021': true,
    'node': true,
  },
  'settings': {
    'react': {
      'version': 'detect',
    },
  },
  'extends': [
    'plugin:react/recommended',
    'plugin:react/jsx-runtime',
    'google',
  ],
  'parser': '@babel/eslint-parser',
  'parserOptions': {
    'ecmaFeatures': {
      'jsx': true,
    },
    'ecmaVersion': 2020,
    'sourceType': 'module',
  },
  'plugins': [
    'react',
    'react-hooks',
    'testing-library',
    'jest',
  ],
  'rules': {
    'require-jsdoc': 0,
    'react/prop-types': 0,
    'prefer-const': 0,
    'linebreak-style': ['error', process.platform === 'win32' ? 'windows' : 'unix'],
    'max-len': [
      'error',
      {'code': 80, 'ignoreStrings': true},
    ],
    'no-restricted-imports': [
      'error',
      {
        paths: [
          {
            name: 'react',
            importNames: ['default'],
            message: 'React default is automatically imported by webpack.',
          },
        ],
      },
    ],
  },
};

Now you will see some minor descriptive errors that you can fix easily:

No need to import 'React'
ESLint: 'default' import from 'react' is restricted. React default is automatically imported by webpack.(no-restricted-imports)

Increase max-len to 120, or break lines in index.ts
ESLint: This line has a length of 121. Maximum allowed is 80.(max-len)

@teafuljs teafuljs deleted a comment from github-actions bot Mar 16, 2022
@github-actions
Copy link

Size Change: -38 B (-1%)

Total Size: 3.52 kB

Filename Size Change
dist/index.js 833 B -9 B (-1%)
dist/index.m.js 846 B -11 B (-1%)
dist/index.modern.js 941 B -4 B (0%)
dist/index.umd.js 899 B -14 B (-2%)

compressed-size-action

@aralroca aralroca marked this pull request as ready for review April 9, 2022 10:58
@aralroca
Copy link
Collaborator Author

aralroca commented Apr 9, 2022

The PR is finnally ready for review 🎉 @danielart

All project moved to TypeScript + add tests for types. Now already passed all tests and library size less than 1kb again.

The version to test in your projects is 0.11.0-canary.2

Before merging the PR it would be great to test in an actual TypeScript project that everything is working correctly with 0.11.0-canary.2. I'm sure it's better than version 0.10 because now there are many more types, but it would be good to check it apart from review the code.

Looking forward to the review 😊 let's see if we finally release version 0.11 soon, having moved the whole project to TypeScript! 🤩

@aralroca aralroca merged commit 11fa158 into master Aug 31, 2022
@aralroca aralroca deleted the aral-migrate-package-to-ts branch August 31, 2022 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants