feat(eslint-plugin): migrate to typescript #2568

G-Rath · 2021-11-26T23:13:56Z

What:

Migrates the @emotion/eslint-plugin to TypeScript, using standard practices and utilities; I've kept the changes to a minimal, but there are some follow-up optimisations that could be done (such as using optional chaining in a few places, turning on es module interop, and using some early returns to dedent some big blocks of code).

This fixed three bugs which I've added tests for, and if I have the time will backport to main since they can be released without typescript being merged.

This also highlighted that jx-import seems to have a bit of a broken/undocumented interface for it's option 😅
It technically accepts a string, but acts (and is tested) as if it'll only ever be passed an object - I've left it as is for now, as changing that could arguably be breaking but I'm happy to help clean that if folks let me know their preferred direction for handling that :)

Why:

Part of #2358

How:

Rewrote the rules in TypeScript 😄

Checklist:

Documentation
Tests
Code complete
Changeset

changeset-bot · 2021-11-26T23:14:02Z

🦋 Changeset detected

Latest commit: f563b29

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@emotion/eslint-plugin	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

G-Rath · 2021-11-26T23:19:42Z

hmm haven't really used Rollup much before (but this could also be an ES modules thing):

Error: 'version' is not exported by packages/eslint-plugin/package.json, imported by packages/eslint-plugin/src/utils.ts

Not quite sure how to tell rollup that package.json is fine?

G-Rath · 2021-11-26T23:21:39Z

Looks like DTSLint is failing for a bunch of packages, which I'd hope is unrelated - btw I've not set it up for this package because usually there's not a lot of reason to emit types for eslint plugins.

Andarist · 2021-11-26T23:59:38Z

Not quite sure how to tell rollup that package.json is fine?

*.json files only export a "default" export - you can't use named exports with them. AFAIK this is also how it works in node. So you just need to do it like here:

emotion/packages/react/src/index.js

Line 2 in 26ded61

import pkg from '../package.json'

Thank you very much for the PR. I'll try to review this soon.

G-Rath · 2021-11-27T00:03:36Z

Ah right thanks - that isn't possible in TS without esModuleInterop enabled, which I recommend enabling but could require adjusting other imports so for now will just use require.

codesandbox-ci · 2021-11-27T00:07:45Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit f563b29:

Sandbox	Source
Emotion	Configuration

codecov · 2021-11-27T00:07:47Z

Codecov Report

Merging #2568 (54bb27d) into ts-migration (908703b) will increase coverage by 0.01%.
The diff coverage is 96.95%.

❗ Current head 54bb27d differs from pull request most recent head f563b29. Consider uploading reports for the commit f563b29 to get more accurate results

Impacted Files	Coverage Δ
packages/css/src/index.ts	`100.00% <ø> (ø)`
packages/eslint-plugin/src/rules/styled-import.ts	`90.90% <90.90%> (ø)`
packages/eslint-plugin/src/rules/pkg-renaming.ts	`91.66% <91.66%> (ø)`
...ges/eslint-plugin/src/rules/import-from-emotion.ts	`94.44% <94.44%> (ø)`
...kages/eslint-plugin/src/rules/syntax-preference.ts	`91.34% <96.96%> (ø)`
packages/css/src/create-instance.ts	`100.00% <100.00%> (ø)`
packages/eslint-plugin/src/rules/jsx-import.ts	`98.00% <100.00%> (ø)`
packages/eslint-plugin/src/rules/no-vanilla.ts	`100.00% <100.00%> (ø)`
packages/eslint-plugin/src/utils.ts	`100.00% <100.00%> (ø)`
packages/eslint-plugin/test/test-utils.ts	`100.00% <100.00%> (ø)`
... and 3 more

srmagura

@G-Rath thank you very much for this work! I made a few minor suggestions.

Thank you for pointing out the discrepancy with the jsx-import options. I created a new issue for that: #2654 (it does not need to be addressed in this PR.)

packages/eslint-plugin/src/rules/no-vanilla.ts

srmagura · 2022-02-19T19:51:03Z

packages/eslint-plugin/src/rules/pkg-renaming.ts

+import { AST_NODE_TYPES } from '@typescript-eslint/experimental-utils'
+import { createRule } from '../utils'
+
+const simpleMappings = new Map<unknown, string>([


Why not Map<string, string>?

Because the type of the source property on ImportDeclaration nodes is Literal, which can be more than just string so we need to allow all of those type of values to be passed:

srmagura · 2022-02-19T19:55:25Z

packages/eslint-plugin/src/rules/jsx-import.ts

+                /* istanbul ignore if */
+                if (jsxImportSourcePragmaComment === null) {
+                  throw new Error(
+                    `Unexpected null when attempting to fix ${context.getFilename()} - please file a github issue at https://github.com/emotion-js/emotion`


nitpick: Use REPO_URL constant?

tbh I could abstract this into an assertion function as well 🤔

srmagura · 2022-02-19T20:01:57Z

packages/eslint-plugin/test/test-utils.ts

@@ -0,0 +1,4 @@
+export const espreeParser: string = require('resolve-from')(


Why is this fancy require stuff necessary? Would it be possible to do a normal import?

Because resolve-from uses a commonjs export (export =) which @babel/plugin-transform-typescript doesn't support out of the box, and this codebase doesn't have esModuleInterop enabled.

I have written a babel plugin that adds support for export = that could be used, but generally esModuleInterop is the better way to go and since this is for tests I thought it would be simpler to just use require here than to muck about with the babel config.

Yeah we should eventually enable esModuleInterop... I've added that to my personal todo list. What you did is totally fine for now.

would it be problematic to enable esModuleInterop as part of this PR? if it is - then don't do it but if it's just a matter for using that option then this could be included here

The trouble is that esModuleInterop actually changes the emitted code so in theory it could result in a breaking change depending on the structure and usage, for users that are not using a transpiler like Babel or TypeScript.

It's unlikely, but I'd prefer to keep this PR to be focused on just eslint-plugin if possible since enabling esModuleInterop doesn't give a strong win here.

srmagura · 2022-02-19T21:54:59Z

.changeset/modern-penguins-play.md

+'@emotion/eslint-plugin': minor
+---
+
+Source code has been migrated to TypeScript. No changes should be required by users


You said you fixed 3 bugs — could you add a changeset for each bug that was fixed? I think doing separate changesets for each is appropriate, but I'm not 100% sure.

Yeah I'm not super familiar with changesets myself, which is why I tried to get away with just one 😅

@srmagura it's been a while so had a bit of trouble remembering the specifics for the bugs, but I believe two of them were the same bug just on different rules so are covered by the one changeset (the erroring when a component had a css prop without a value), and have added a change set for the remaining bug.

Co-authored-by: Sam Magura <srmagura@gmail.com>

srmagura

@Andarist This is ready for merge AFAIK.

Thanks @G-Rath!

Andarist · 2022-03-18T20:46:33Z

packages/eslint-plugin/test/rules/jsx-import.test.ts

+      code: `
+        /** @jsx jsx */
+        import {jsx} from '@emotion/react'
+        let ele = <div css />


is this one considered to be valid though? This is equivalent to css={true} and that doesn't quite make sense

I think you're right, but this PR is meant to be focusing on converting the existing code to TypeScript, so I prefer to do any behaviour changes like this in follow up PRs.

The reason this is here in the first place is because it covers a new condition I have to add in order to make the code valid with TypeScript, since it revealed an unchecked nullish value.

So I guess this is sort of valid for this particular rule - although not valid in the general sense. I'm not sure in which rule we should validate this though

Andarist · 2022-03-18T20:48:15Z

packages/eslint-plugin/test/rules/syntax-preference.test.ts

@@ -162,6 +164,10 @@ ruleTester.run('syntax-preference (object)', rule, {
    {
      code: `const Foo = () => <div css={css({ color: 'hotpink' })} />`,
      options: ['object']
+    },
+    {
+      code: `const Foo = () => <div css />`,


same doubt as here

Andarist · 2022-03-18T20:55:12Z

packages/eslint-plugin/test/rules/jsx-import.test.ts

+      ],
+      output: `
+/** @jsx jsx */
+/** @jsx emotion.jsx */


This one seems like a new test case. Was this a bug and it got fixed here? From what I understand the issue could be about the namespace import being used in a file with an invalid pragma for such an import. I've skimmed through the related code and I don't see any particular changes that would fix this - although I'm not super familiar with this code, so I could have missed the fix.

This should have been for either an actual bug or to ensure coverage, but I can't get it to fail so happy to remove it if you like

Andarist · 2022-03-18T21:53:50Z

By the way - thank you very much for this PR ❤️

G-Rath added 9 commits November 27, 2021 09:54

feat(eslint-plugin): add standard utils & dependencies for typescript

328872e

feat(eslint-plugin): convert no-vanilla to typescript

cd5b441

feat(eslint-plugin): convert styled-import to typescript

952b43e

feat(eslint-plugin): convert import-from-emotion to typescript

44138c1

feat(eslint-plugin): convert syntax-preference to typescript

8057d17

feat(eslint-plugin): convert pkg-renaming to typescript

c5cf134

feat(eslint-plugin): convert jsx-import to typescript

40f1f80

feat(eslint-plugin): move index to typescript

cd72d3e

feat(eslint-plugin): add changeset

9431e92

fix(eslint-plugin): use require to get package version

cc02094

srmagura self-requested a review February 19, 2022 19:39

srmagura mentioned this pull request Feb 19, 2022

eslint-plugin/jsx-import: Options schema accepts string but not really #2654

Open

srmagura requested changes Feb 19, 2022

View reviewed changes

G-Rath and others added 3 commits February 20, 2022 12:23

fix(eslint-plugin): adjust rule description

ed99a02

Co-authored-by: Sam Magura <srmagura@gmail.com>

fix(eslint-plugin): use REPO_URL constant everywhere

8f77c79

chore(eslint-plugin): add change sets for other bugs

54bb27d

G-Rath requested a review from srmagura February 19, 2022 23:44

srmagura approved these changes Feb 20, 2022

View reviewed changes

Andarist reviewed Mar 18, 2022

View reviewed changes

Merge branch 'ts-migration' into migrate-eslint-plugin-to-typescript

10f15ec

Andarist added 4 commits April 10, 2022 10:15

nit

daa1402

Tweak changeset

bf77e35

Report empty css attribute as invalid in syntax-preference rule

5a83933

Tweak changesets

f563b29

Andarist merged commit 304f7e3 into emotion-js:ts-migration Apr 11, 2022

github-actions bot mentioned this pull request May 17, 2024

Version Packages #3188

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eslint-plugin): migrate to typescript #2568

feat(eslint-plugin): migrate to typescript #2568

G-Rath commented Nov 26, 2021

changeset-bot bot commented Nov 26, 2021 •

edited

Loading

G-Rath commented Nov 26, 2021

G-Rath commented Nov 26, 2021

Andarist commented Nov 26, 2021

G-Rath commented Nov 27, 2021

codesandbox-ci bot commented Nov 27, 2021 •

edited

Loading

codecov bot commented Nov 27, 2021 •

edited

Loading

srmagura left a comment

srmagura Feb 19, 2022

G-Rath Feb 19, 2022

srmagura Feb 19, 2022

G-Rath Feb 19, 2022

srmagura Feb 19, 2022

G-Rath Feb 19, 2022 •

edited

Loading

srmagura Feb 19, 2022

Andarist Mar 18, 2022

G-Rath Mar 19, 2022

srmagura Feb 19, 2022

G-Rath Feb 19, 2022

G-Rath Feb 19, 2022

srmagura left a comment

Andarist Mar 18, 2022

G-Rath Mar 19, 2022

Andarist Apr 10, 2022

Andarist Mar 18, 2022

Andarist Mar 18, 2022

G-Rath Mar 19, 2022

Andarist commented Mar 18, 2022

		@@ -0,0 +1,4 @@
		export const espreeParser: string = require('resolve-from')(

feat(eslint-plugin): migrate to typescript #2568

feat(eslint-plugin): migrate to typescript #2568

Conversation

G-Rath commented Nov 26, 2021

changeset-bot bot commented Nov 26, 2021 • edited Loading

🦋 Changeset detected

G-Rath commented Nov 26, 2021

G-Rath commented Nov 26, 2021

Andarist commented Nov 26, 2021

G-Rath commented Nov 27, 2021

codesandbox-ci bot commented Nov 27, 2021 • edited Loading

codecov bot commented Nov 27, 2021 • edited Loading

Codecov Report

srmagura left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

G-Rath Feb 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srmagura left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andarist commented Mar 18, 2022

changeset-bot bot commented Nov 26, 2021 •

edited

Loading

codesandbox-ci bot commented Nov 27, 2021 •

edited

Loading

codecov bot commented Nov 27, 2021 •

edited

Loading

G-Rath Feb 19, 2022 •

edited

Loading