-
Notifications
You must be signed in to change notification settings - Fork 538
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
Remove Babel #1045
Remove Babel #1045
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/primer/primer-components/l3cbt4mkk |
@@ -1,13 +0,0 @@ | |||
const shared = { | |||
__DEV__: "process.env.NODE_ENV !== 'production'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: If this is still needed, provide it via @rollup/plugin-replace (for yarn run dist:rollup
builds) and via sed
or similar (for yarn run dist:transpile:*
builds).
return ['babel-plugin-transform-replace-expressions', {replace: defines[env]}] | ||
} | ||
|
||
const sharedPlugins = ['macros', 'preval', 'add-react-displayname', 'babel-plugin-styled-components', '@babel/plugin-proposal-nullish-coalescing-operator'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Review these plugins and supply any missing functionality.
typescript-plugin-styled-components
was installed to replacebabel-plugin-styled-components
- Nullish Coalescing is built into TypeScript
When push comes to shove and you just _really_ need to add a custom CSS rule, you can do that with the `css` prop. Please don't abuse this :) | ||
|
||
``` | ||
<Box css='border-bottom-right-radius: 0px' /> | ||
|
||
``` | ||
|
||
Please note that you will need to have the **[styled-components babel plugin](https://www.styled-components.com/docs/tooling#babel-plugin)** set up in your project in order to use the `css` prop. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Verify css
still works.
exports.onCreateWebpackConfig = ({actions, plugins, loaders, getConfig}) => { | ||
const config = getConfig() | ||
// Add our `__DEV__` and `process.env.NODE_ENV` defines | ||
config.plugins.push(plugins.define(defines[process.env.NODE_ENV || 'development'])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check out the note above: https://github.com/primer/components/pull/1045/files#r574817860
"dist:transpile:cjs": "rm -rf lib && cross-env BABEL_MODULE=commonjs babel src --out-dir lib --extensions '.js,.ts,.jsx,.tsx'", | ||
"dist:transpile:esm": "rm -rf lib-esm && babel src --out-dir lib-esm --extensions '.js,.ts,.jsx,.tsx'", | ||
"dist:types": "rm -rf types && tsc", | ||
"dist:transpile:cjs": "rm -rf lib && tsc -p tsconfig.json --outDir lib --module commonjs", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Do we need to continue to publish transpiled versions of PRC, or can we publish the TS alone?
-
TODO: Review the transpiled output for any obvious issues.
"babel-plugin-styled-components": "1.10.7", | ||
"babel-plugin-transform-replace-expressions": "0.2.0", | ||
"babel-polyfill": "6.26.0", | ||
"acorn-jsx": "5.3.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: (Optional) Stub acorn-jsx
types, similar to what we did for @styled-system/prop-types
"rollup-plugin-terser": "5.3.0", | ||
"rollup-plugin-visualizer": "4.0.4", | ||
"semver": "7.3.2", | ||
"styled-components": "4.4.0", | ||
"typescript": "4.1.3" | ||
"typescript": "4.1.3", | ||
"typescript-plugin-styled-components": "1.4.4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: This is currently unused. Review the typescript-plugin-styled-components
docs and call this appropriately.
import commonjs from '@rollup/plugin-commonjs' | ||
import resolve from '@rollup/plugin-node-resolve' | ||
import {terser} from 'rollup-plugin-terser' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Figure out why JSX syntax was confusing terser
(it shouldn’t, since terser
is used in other GitHub TypeScript projects’ rollup
configs), then restore it.
terser(), | ||
visualizer({sourcemap: true}) | ||
] | ||
const plugins = [resolve({extensions}), typescript(), commonjs(), visualizer({sourcemap: true})] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Double-check plugin order.
import {COMMON} from '../constants' | ||
import 'jest-styled-components' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: The jest-styled-components
docs suggest this usage (i.e. explicitly importing it in every test file), but we ought to be able to import it once, in a shared/common file. We just need its toHaveStyleRule
function type monkey-patch on jest
.
import {render as HTMLRender, cleanup} from '@testing-library/react' | ||
import {axe, toHaveNoViolations} from 'jest-axe' | ||
import 'babel-polyfill' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
INFO: I expect this will reduce bundle size for downstream consumers.
"outDir": "./types", | ||
"emitDeclarationOnly": true, | ||
"strict": true, | ||
"module": "esnext", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
INFO: Several of these options are overriden in yarn run dist:transpile:*
. I updated them here to align with our other TypeScript projects, though I’m open to revisions, since TypeScript libraries like this may have different needs vs TypeScript app code.
…thub.com/primer/react into /issues/1045/actionlist-focus-navigation
…thub.com/primer/react into /issues/1045/actionlist-focus-navigation
Part of #970
We build other TypeScript projects with
tsc
, notbabel
. This PR replacesbabel
withtsc
.Screenshots
Please provide before/after screenshots for any visual changes
Merge checklist
index.d.ts
) if necessaryTake a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.