[v0.36] fix(babel-hooks): Import babel hooks correctly for console, exec and dm up commands #3231

dac09 · 2021-08-11T10:35:32Z

Critical fix for v36 release - to be able to run rw console, rw exec and rw dm commands.

The issue was the removal of .babel.rc.js from the api side here: #3187 so these commands could not regsiter the babel hook

What does this do?

Fixes how the above commands register the babel hook
Registering babel hooks now moved to @redwoodjs/internal, so they all work consistently
Makes sure babel hooks are only registered inside handlers (in particular data-migrate) - this might have been why prerender was failing in certain situations
Makes change in Use user babel.config.js on api side. #3187 backwards compatible i.e. users no longer need to remove api/.babelrc.js - we simply ignore it

What this PR won't do

Modify anything on the web side, apart from how prerender loads it's babel config to be consistent and provider a pattern for future use.
Refactor how prerender exports stuff, now that the babel register issue may be resolved. I will do this separately

Outstanding

Update tests
Investigate prerender failure

up commands

packages/internal/src/babel/api.ts

dac09 · 2021-08-11T11:11:01Z

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

+    ['@babel/plugin-transform-typescript', undefined, 'rwjs-babel-typescript'],
+    [
+      require('@redwoodjs/core/dist/babelPlugins/babel-plugin-redwood-src-alias')
+        .default,


@peterp

Notice I had to add .default because I think.... with the datamigrate hook was doing the transform for us (accidentally)

Did you manage to fix this by making it inline? Super weird behaviour, it should be ignore node_modules.

Not sure what you mean by inline - as in require them before the plugin block?

You mentioned that you changed something in datamigrate when you registered the hook, is this still a problem?

I think it was the opposite actually - because the cli was registering the hooks, we didn't need to do the .default. It is very strange to me as well...

packages/cli/src/commands/dataMigrate/up.js

dac09 · 2021-08-11T11:13:53Z

@peterp any thoughts on how to add more tests for these?

packages/cli/src/commands/destroy/scaffold/__tests__/scaffold.test.js

…ed-babel-location-require-hook * 'main' of github.com:redwoodjs/redwood: webpack and misc upgrades (#3235) upgrade Prisma v2.29.0 (#3233) Fix API sourcemaps (babel+esbuild) for debugging (#3230) set canary as default (#3228) Fix issue where Clerk client is sometimes unset (#3224)

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

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

peterp

Looking good ⚡

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

peterp · 2021-08-12T14:58:55Z

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

+  if (fs.existsSync(p)) {
+    return p
+  } else {
+    return undefined


There's a difference between returning false and undefined. In the false case it doesn't search for any additional configuration - essentially turning it off, in the undefined case it invokes the traditional methods where it searches for the babel config.

We actually don't want it to search for .babelrc.js because of the way it extends the base configuration. (It would mean duplicate plugins in a lot of cases) and it would be difficult to track down.

Ah yes... sorry I should change this to false. I had it as undefined because I was using extends.

I read the docs, it might not actually be searching for .babelrc.js, but I would still prefer it to be turned off completely.

Peter, so using false is different from using undefined. Using false just makes it fail... my suspicion babel register behaves differently to babel transform.

What I'm not loving about this PR is that it feels like it's introducing more uncertainty in a process that I would love to be solid. This is the outcome I'm trying to achieve:

So this is what I want:

I want people to delete api/.babelrc.js, or to convert it to api/babel.config.js and remove the "extends" part.

I want this to be a breaking change if you have custom babel transformations. The reason I want it to be a breaking change is because I want to discourage people from adding their own babel transformations because I would like to move our own transformations to a faster toolchain and would not like babel to make that process difficult.

Can we work around making that a realistic goal in this PR instead of muddying the water? And is it something that we can have tests for?

Test that <side>/.babelrc.js is not imported

Test that <side>/babel.config.json can be imported if it's available.

^^ @thedavidprice Does this reasoning sound solid to you?

@peterp The items 1 and 2 above regarding your plan to deprecate and why we should deprecate are SuperImportant™. I'm 100% in agreement with 2 and moving forward with implementation on 1. (But not in this PR, see "next steps" below.)

Instead of this information being buried here, however, what would be really helpful is having this plan communicated in an Issue for visibility and alignment. I'm very interested in the move to a "faster toolchain" (tell me more!), I just don't know what that means, when it would happen, what it will require, etc.

I think I understand what you mean by "introducing more uncertainty". In this PR, we are deprecating bablerc, but because it won't throw an app error anymore you are concerned it will be confusing, correct? I could go either way. Regardless, I think we can resolve this with 1) good Release Note communication and 2) a two-step process to full deprecation (e.g. move forward with v0.36 and throw if present in v0.37).

Next Steps

We've been 2 weeks on this bug as a critical-path blocker to releasing v0.36. The code here is an improvement and, most importantly, resolves the release-blocking bugs.

I'll approve and merge, then cut the v0.36 release branch

I'll open an Issue with these next steps for v0.37

PP, I'll leave next steps regarding communicating a plan for item 2 above (babel + faster toolchain) if applicable

Just to close the loop on this -- we spoke offline.

The PR where I removed the .babelrc.js files (#3187) includes a deprecation notice for the release notes. I'll update the follow on PR #3245 with a bit more information about next steps.

And the current Release Notes draft is very clear about the Breaking change + Code Mods regarding I want people to delete api/.babelrc.js, or to convert it to api/babel.config.js and remove the "extends" part. — says this explicitly.

Sound good?

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

packages/internal/src/build/api.ts

packages/internal/package.json

peterp · 2021-08-12T15:32:49Z

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

+    ['@babel/plugin-transform-typescript', undefined, 'rwjs-babel-typescript'],
+    [
+      require('@redwoodjs/core/dist/babelPlugins/babel-plugin-redwood-src-alias')
+        .default,


Did you manage to fix this by making it inline? Super weird behaviour, it should be ignore node_modules.

peterp · 2021-08-12T15:57:53Z

packages/prerender/src/runPrerender.tsx

+            'babel-plugin-module-resolver',
+            {
+              alias: {
+                src: rwWebPaths.src,
+              },
+              root: [getPaths().web.base],
+              // needed for respecting users' custom aliases in web/.babelrc
+              // See https://github.com/tleunen/babel-plugin-module-resolver/blob/master/DOCS.md#cwd
+              cwd: 'babelrc',
+              loglevel: 'silent', // to silence the unnecessary warnings
+            },


Does the rw alias plugin I wrote not work in this case?

I suggest we don't use that here for now - because there's a chance people may have custom aliases setup (like Burnsy).

My suggestions: keep as is, and in a later PR - we add support for specifying your own aliases, and ask users to migrate to this new system.

Maybe we can be clever, and just read their tsconfig/jsconfig instead of them having to specify it somewhere else.

And get rid of the module-resolver plugin all together ☠️

Using ours would not invalidate them using theirs.

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

peterp · 2021-08-12T16:00:57Z

packages/cli/src/commands/exec.js

      [
        'babel-plugin-module-resolver',
        {
          alias: {
            $api: getPaths().api.base,
          },
        },
+        'exec-$api-module-resolver',
      ],
    ],


I would kick this babel plugin out and try to use the one I wrote.

Mind pointng me to the plugin please? Didn't realise you added it in 😃

Here you go! I really struggled with the other one:

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

Lines 88 to 94 in 8e3f9c6

[

require('@redwoodjs/core/dist/babelPlugins/babel-plugin-redwood-src-alias'),

{

srcAbsPath: rwjsPaths.api.src,

},

'rwjs-babel-src-alias',

],

It does not support multiple alias, just "srcAbsPath", but it makes the files relative in the resulting ast.

Oh, I guess in this case it WOULD not work. Sorry, super tired, but for pre-render it would work

Ah.... I think we should leave this as is for now in that case. Because one of the changes I made a while ago was to support user defined aliases (like in Burnsy's project).

What do you think of leaving this for now (to make it non-breaking) - and switch everything over (including babel-preset) and remove babel-plugin-module-resolver once we get to pre-transpiling web?

…ed-babel-location-require-hook * 'main' of github.com:redwoodjs/redwood: [v0.36] Adds ability to test api functions (#3207)

dac09 · 2021-08-12T17:33:27Z

All comments addressed!

thedavidprice

Approved. Moved next steps here for v0.37 "Full deprecation of .babelrc.js" #3245

dac09 added 2 commits August 11, 2021 11:29

fix(babel-hooks): Import babel hooks correctly for console, exec and dm

115fc24

up commands

Change prerender to import babel settings from internal too

9f409f5

redwoodjs-bot bot added this to New issues in Current-Release-Sprint Aug 11, 2021

dac09 marked this pull request as draft August 11, 2021 10:37

peterp reviewed Aug 11, 2021

View reviewed changes

packages/internal/src/babel/api.ts Outdated Show resolved Hide resolved

dac09 commented Aug 11, 2021

View reviewed changes

packages/internal/src/babel/api.ts Outdated Show resolved Hide resolved

dac09 commented Aug 11, 2021

View reviewed changes

packages/internal/src/babel/api.ts Outdated Show resolved Hide resolved

dac09 added 2 commits August 11, 2021 12:06

Move folder | Fix imports

bc78a18

Silly vscode refactor

6365987

dac09 commented Aug 11, 2021

View reviewed changes

packages/cli/src/commands/dataMigrate/up.js Show resolved Hide resolved

Remove babel-core mocks from tests

e65f8b7

dac09 commented Aug 11, 2021

View reviewed changes

packages/cli/src/commands/destroy/scaffold/__tests__/scaffold.test.js Show resolved Hide resolved

dthyresson mentioned this pull request Aug 11, 2021

Improved yarn rw console by adding persistent history #3221

Merged

Typo in babelrc file

bbe7b62

thedavidprice changed the title ~~fix(babel-hooks): Import babel hooks correctly for console, exec and dm up commands~~ [v0.36] fix(babel-hooks): Import babel hooks correctly for console, exec and dm up commands Aug 11, 2021

thedavidprice added this to the next-release-priority milestone Aug 11, 2021

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

thedavidprice moved this from In progress (priority) to v0.36 in Current-Release-Sprint Aug 11, 2021

dac09 added 3 commits August 12, 2021 13:22

import -> require babel/register

5809004

Small refactor | Make babel userconfig loading non breaking

034cfd7

dac09 marked this pull request as ready for review August 12, 2021 12:50

dac09 requested a review from thedavidprice August 12, 2021 14:43