Removes babel configuration from users project #3719

dac09 · 2021-11-10T21:08:42Z

This PR tries to achieve:
a) Move all of the babel related config required for running anything within redwood to inside the framework
Why? This sets us up for being able to change things internally between versions, and also longer term move to something like SWC when its ready
b) Leaves support for bringing your own babel plugins, but only using babel.config.js per side

DX

Warn users if .babelrc is present, that it'll be ignored (exits with error)
https://user-images.githubusercontent.com/1521877/142239626-8a333aa7-8d50-4583-b538-b0e5debba2a3.mp4
~~Warn users if babel.config.js is present, that this will be removed and may not be supported in future version~~

Web

Api

Both

rw lint

Other

Check Including babel plugin for web side
Check Including babel plugin for api side
Can we separate framework and project eslint config
deploy Vercel
deploy Netlify

Load all babel config from rwjs/interal

dac09 · 2021-11-11T17:44:39Z

@peterp just wanted to confirm... if I can get the web side working too, we want to remove the root babel.config.js right?

dac09 · 2021-11-11T17:57:52Z

Status: currently stuck with the tailwind setup command, because eslint-config depends on presence of babel.config.js: https://github.com/redwoodjs/redwood/blob/main/packages/eslint-config/shared.js#L20

~~wondering whether I introduce a dependency to internal here just to import babel config. I'm also not sure how to configure it independantly for the api and the web side.~~

~~@jtoar - any thoughts on this?~~

Resolved

…est-fun * 'main' of github.com:redwoodjs/redwood: Throw error on missing route param. (redwoodjs#3714) Regenerate lock file(s) (redwoodjs#3722) update yarn.lock udpate yarn.lock v0.38.1 update yarn.lock Have Redwood figure out in what order tables need to be cleared during scenario teardown (redwoodjs#3716) Base64 encode smaller required images during prerender (redwoodjs#3705) Have Redwood figure out in what order tables need to be cleared during scenario teardown (redwoodjs#3716) Base64 encode smaller required images during prerender (redwoodjs#3705) Fixes syntax error Firebase Auth template initializeApp (redwoodjs#3709) Fixes UserInputError mesage display in FormError (redwoodjs#3704) dbAuth: Resolved typos and inconsistencies in templates (redwoodjs#3698) fixed styling post-setup-tailwind command (redwoodjs#3696) More router tests. (redwoodjs#3718) Fix Fastify link. Remove Codesee integration. (redwoodjs#3717) Teach named route function to handle glob routes (redwoodjs#3702)

jtoar · 2021-11-14T22:06:42Z

@dac09 The indirect babel dependency in the tailwind setup command is through execa, and it’s just to format a file. We could try doing it programmatically with prettier instead like we do in some of the codemods: https://github.com/redwoodjs/redwood/blob/main/packages/codemods/src/lib/prettify.ts. When I originally wrote this I probably just copied the execa call from another part of the code base, but we should start moving away from it for sure.

If the call to eslint breaks in the tailwind setup command, does yarn rw lint still work?

dac09 · 2021-11-14T23:07:21Z

Managed to make progress on this @jtoar - but I'm now back to getting failures (Babel related) when jest web is run with runInBand.

Here's the worst part - it doesn't always fail (passes in e2e).

jtoar · 2021-11-14T23:52:49Z

Always one thing after another with these things!

We only had the web side runInBand because yarn rw test web broke unexpectedly right before a release (something about TextDecoder). We always had the api side running in band.

Is the web side still broken even without runInBand?

peterp · 2021-11-17T21:26:20Z

packages/internal/src/build/babel/common.ts

+    // Not sure about this one:
+    // Do we need this?
+    // [
+    //   '@babel/plugin-transform-runtime',
+    //   {
+    //     // https://babeljs.io/docs/en/babel-plugin-transform-runtime/#core-js-aliasing
+    //     // Setting the version here also requires `@babel/runtime-corejs3`
+    //     corejs: { version: 3, proposals: true },
+    //     // https://babeljs.io/docs/en/babel-plugin-transform-runtime/#version
+    //     // Transform-runtime assumes that @babel/runtime@7.0.0 is installed.
+    //     // Specifying the version can result in a smaller bundle size.
+    //     version: RUNTIME_CORE_JS_VERSION,
+    //   },
+    // ],


🤷 I guess a test would be really helpful here to accurately describe what this does.

@dac09 Unless this is throwing an error, I'd really prefer we not change (in this case remove) babel config at this stage in our roadmap. We know it's an optimization step, but we don't know how/if it could have downstream affect on things like Netlify deploy, etc.

tl;dr:
unless we have a strong case to remove this, we should leave it in.

I'm of the opposite opinion - I think it's better not to include this, unless we're also including Babel/runtime as a dependency on the rwjs/api. These two things must go hand in hand - as we learnt from one our releases a while ago.

I'm pretty sure the fact that this was still in the root Babel.config was wrong - and ignored for builds anyway. So adding it here would be a change 😂, confusing I know!

Yep as I mentioned @thedavidprice this needs @babel/runtime dep if we use this plugin. See here: https://babeljs.io/docs/en/babel-plugin-transform-runtime

~~We can evaluate later on whether we should include this, but to maintain the same configuration as we've had past couple of releases, we should not include this~~

I've spent some time writing unit tests, to understand exactly what's happening and I finally think I understand (a little) more of whats been happening, and verified my assumptions. I'll update this, and include it, but also will include relevant dependencies on the rwjs/api package.

Ah, a lot going on here. Also, I didn't correctly understand the relationship with @babel/runtime

I'm pretty sure the fact that this was still in the root Babel.config was wrong - and ignored for builds anyway. So adding it here would be a change 😂, confusing I know!

^^ Got it. And definitely an important consideration!

Happy to discuss real-time later this morning (Pacific time)! My primary concern is keeping babel config consistent with this PR. And now I better understand why you were making this (apparent) change.

OK Update on this, thanks to @dthyresson for rubber ducking with me. We have 3 options here:

Option A: remove polyfill plugin, only have transform runtime

We remove the polyfill plugin babel-plugin-polyfill-corejs3 and include babel-plugin-transform-runtime. We will also need to change our framework config to include babel-plugin-transform-runtime, because otherwise it won't use the correct module

We then only have @babel/runtime-corejs3 as a dependency on the api package.json.

Why?

It seems the polyfill-corejs3 doesn't cover all cases of polyfilling. e.g. Array.at()

polyfill plugin will add imports, where it isn't required (e.g. try doing Promise.any)

We want to minimise the number of these polyfill dependencies, so the framework config must match the project config

Option B: accept polyfill-corejs3 misvehaviour

Include both plugins

We accept that this plugin will sometimes misbehave

We include both corejs and @babel/runtime-corejs3 as dependencies

Trade off is that we are adding both these dependencies at runtime (worth about 1.2MB) in the api side. The web side should be fine since we're bundling anyway

Option C: not include transform-runtime plugin

Only have polyfill-corejs3 plugin

Have corejs as dependency in rwjs/api

This is effectively what we have right now

Accept that some things won't be polyfilled e.g. Array.at()

I don't think all of this really affects the web side all that much - since the webside uses preset-env anyway

If I recall correctly (there's a 50/50 chance), the babel-plugin-polyfill-corejs3 was added to correct for ESBuild behavior.

y/n/maybe?

Pushing up option A at the moment, until we discuss. See commit with tests: 18eb8df (#3719)

packages/internal/src/build/babel/web.ts

peterp

LGTM

thedavidprice

Wowza @dac09, this here is a ton of great work. Thank you 🚀

Only question I have in addition to my comments is whether or not the middleware check for Babel files should also be added to the rw build command?

thedavidprice · 2021-11-18T00:52:27Z

packages/internal/src/build/babel/common.ts

+    // Not sure about this one:
+    // Do we need this?
+    // [
+    //   '@babel/plugin-transform-runtime',
+    //   {
+    //     // https://babeljs.io/docs/en/babel-plugin-transform-runtime/#core-js-aliasing
+    //     // Setting the version here also requires `@babel/runtime-corejs3`
+    //     corejs: { version: 3, proposals: true },
+    //     // https://babeljs.io/docs/en/babel-plugin-transform-runtime/#version
+    //     // Transform-runtime assumes that @babel/runtime@7.0.0 is installed.
+    //     // Specifying the version can result in a smaller bundle size.
+    //     version: RUNTIME_CORE_JS_VERSION,
+    //   },
+    // ],


@dac09 Unless this is throwing an error, I'd really prefer we not change (in this case remove) babel config at this stage in our roadmap. We know it's an optimization step, but we don't know how/if it could have downstream affect on things like Netlify deploy, etc.

tl;dr:
unless we have a strong case to remove this, we should leave it in.

thedavidprice · 2021-11-18T00:56:50Z

packages/internal/src/build/babel/api.ts

+  getCommonPlugins,
+} from './common'
+
+const TARGETS_NODE = '12.16'


Is this still the "correct" target? And/or when will we know it's time to bump?

Just for future discussion.

thedavidprice · 2021-11-18T01:21:05Z

packages/eslint-config/framework.js

+const path = require('path')
+
+const findUp = require('findup-sync')
+
+const findBabelConfig = (cwd = process.cwd()) => {
+  const configPath = findUp('babel.config.js', { cwd })
+  if (!configPath) {
+    throw new Error(`Eslint-parser could not find a "babel.config.js" file`)
+  }
+  return configPath
+}
+
+// Just configure the babel options
+// The framework is configured with babel.config.js
+// While projects are configured programatically from rwjs/internal
+module.exports = {
+  extends: path.join(__dirname, 'shared.js'),
+  parserOptions: {
+    babelOptions: {
+      configFile: findBabelConfig(),
+    },
+  },
+}


@dac09 could all this code live in ./.eslintrc.js instead of a separate file here?

Nope, because of the "findUp" dependency

But will you need findUp since they're both in root?

Hmmm I don't know, I'll try, but I wanted to minimise the changes here... I find lint configuration boring and impossible to debug.

Understood. When I was tracing the changes and saw the files "shared" and "framework", I assumed two things: 1) there would be a "project" and 2) there would be eslint config in framework. When I saw that this file is specifically handling associated babel config for the framework, that's what got me thinking about whether or not it could be simplified further.

So all that mostly for discussion. Maybe what all this really needs is documentation in the package README.

packages/core/config/webpack.common.js

packages/testing/config/jest/web/RedwoodWebJestEnv.js

packages/testing/config/jest/web/index.js

dac09 · 2021-11-18T11:43:56Z

Only question I have in addition to my comments is whether or not the middleware check for Babel files should also be added to the rw build command?

done, I dont know if its useful, but I suppose it doesn't hurt!

Also change framework config to make sure corejs isn't imported

thedavidprice · 2021-11-19T18:57:07Z

I've moved Framework ESLint config into root .eslintrc.js and confirmed it's working. It's a minor change, but it's much easier for my brain to parse where the associated settings are within one file in root. Added comments to explain as well.

Waiting for CI to pass, will then merge.

dac09 added 3 commits November 9, 2021 17:10

WIP: playing with presets and jest babel config

ddfff6f

Only set preset env for jest

e16384b

Remove all babelconfig from users project, including

e2b2ba7

Load all babel config from rwjs/interal

redwoodjs-bot bot added this to New issues in Current-Release-Sprint Nov 10, 2021

redwoodjs-bot bot assigned thedavidprice Nov 10, 2021

redwoodjs-bot bot added the triage/processing label Nov 10, 2021

dac09 added 3 commits November 10, 2021 21:12

Whoops deleted the wrong rc

56793eb

Fix apiSide register

057b307

Fix prerendering

908c6db

thedavidprice self-requested a review November 10, 2021 22:57

thedavidprice assigned dac09 and unassigned thedavidprice Nov 10, 2021

thedavidprice removed the triage/processing label Nov 10, 2021

thedavidprice added this to the next-release-priority milestone Nov 10, 2021

redwoodjs-bot bot moved this from New issues to In progress (priority) in Current-Release-Sprint Nov 10, 2021

thedavidprice moved this from In progress (priority) to v0.39 (locked) in Current-Release-Sprint Nov 10, 2021

thedavidprice linked an issue Nov 10, 2021 that may be closed by this pull request

[v0.37] Full deprecation of .babelrc.js #3245

Closed

Supsource approved these changes Nov 11, 2021

View reviewed changes

dac09 added 4 commits November 12, 2021 13:32

Try programatically generating babel config for project

cc2d774

Fixes for linter config

5bff963

A few tweaks

40de3e3

Supsource approved these changes Nov 12, 2021

View reviewed changes

dac09 added 2 commits November 12, 2021 16:27

Config to configFile

fcf55aa

Lint the linter config...

a8e3dc5

peterp reviewed Nov 17, 2021

View reviewed changes

packages/internal/src/build/babel/web.ts Show resolved Hide resolved

peterp approved these changes Nov 17, 2021

View reviewed changes

Current-Release-Sprint automation moved this from v0.39 (locked) to Ready to merge Nov 17, 2021

thedavidprice added the release:breaking This PR is a breaking change label Nov 18, 2021

thedavidprice requested changes Nov 18, 2021

View reviewed changes

dac09 added 4 commits November 18, 2021 11:29

Add babelrc check to build command

48837e2

Add comment to explain presetEnv flag

6437507

Consistent test pattern for cell and routes overrides

61ed774

Undo web jest env changes

6a33ba1

dac09 added 3 commits November 18, 2021 11:44

Merge branch 'main' into feat/babel-jest-fun

f5febfc

Make fast-glob explicit cli dependency

b0f4eac

Bump eslint config dep to 0.38.3 too

ff0ffb6

dac09 force-pushed the feat/babel-jest-fun branch 3 times, most recently from 18eb8df to 2e8954a Compare November 18, 2021 17:00

Option A: remove polyfill plugin, only have transform-runtime

e8415b1

Also change framework config to make sure corejs isn't imported

dac09 force-pushed the feat/babel-jest-fun branch from 2e8954a to e8415b1 Compare November 18, 2021 17:03

thedavidprice added 2 commits November 19, 2021 10:52

move framework ESlint settings 100% to ./.eslintrc.js

5d069e8

Merge branch 'main' into feat/babel-jest-fun

9b9cdc6

thedavidprice approved these changes Nov 19, 2021

View reviewed changes

thedavidprice merged commit 03cb1fc into redwoodjs:main Nov 19, 2021

Current-Release-Sprint automation moved this from Ready to merge to Done Nov 19, 2021

redwoodjs-bot bot modified the milestones: next-release-priority, next-release Nov 19, 2021

thedavidprice modified the milestones: next-release, v0.39.0 Nov 20, 2021

simoncrypta mentioned this pull request May 13, 2022

Fix yarn rw build web --perf option #5527

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removes babel configuration from users project #3719

Removes babel configuration from users project #3719

dac09 commented Nov 10, 2021 •

edited by thedavidprice

dac09 commented Nov 11, 2021

dac09 commented Nov 11, 2021 •

edited

jtoar commented Nov 14, 2021

dac09 commented Nov 14, 2021

jtoar commented Nov 14, 2021

peterp Nov 17, 2021

thedavidprice Nov 18, 2021

dac09 Nov 18, 2021 •

edited

dac09 Nov 18, 2021 •

edited

thedavidprice Nov 18, 2021

dac09 Nov 18, 2021 •

edited

thedavidprice Nov 18, 2021

dac09 Nov 18, 2021

dac09 Nov 18, 2021 •

edited

peterp left a comment

thedavidprice left a comment

thedavidprice Nov 18, 2021

thedavidprice Nov 18, 2021

thedavidprice Nov 18, 2021

dac09 Nov 18, 2021

thedavidprice Nov 18, 2021

dac09 Nov 18, 2021

thedavidprice Nov 18, 2021

dac09 commented Nov 18, 2021

thedavidprice commented Nov 19, 2021

Removes babel configuration from users project #3719

Removes babel configuration from users project #3719

Conversation

dac09 commented Nov 10, 2021 • edited by thedavidprice

dac09 commented Nov 11, 2021

dac09 commented Nov 11, 2021 • edited

jtoar commented Nov 14, 2021

dac09 commented Nov 14, 2021

jtoar commented Nov 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dac09 Nov 18, 2021 • edited

Choose a reason for hiding this comment

dac09 Nov 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dac09 Nov 18, 2021 • edited

Choose a reason for hiding this comment

Option A: remove polyfill plugin, only have transform runtime

Option B: accept polyfill-corejs3 misvehaviour

Option C: not include transform-runtime plugin

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dac09 Nov 18, 2021 • edited

Choose a reason for hiding this comment

peterp left a comment

Choose a reason for hiding this comment

thedavidprice 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

Choose a reason for hiding this comment

dac09 commented Nov 18, 2021

thedavidprice commented Nov 19, 2021

dac09 commented Nov 10, 2021 •

edited by thedavidprice

dac09 commented Nov 11, 2021 •

edited

dac09 Nov 18, 2021 •

edited

dac09 Nov 18, 2021 •

edited

dac09 Nov 18, 2021 •

edited

dac09 Nov 18, 2021 •

edited