[benchmark] Set intended environment#25402
Conversation
| options: { cacheDirectory: true, cwd: workspaceRoot }, | ||
| options: { | ||
| cacheDirectory: true, | ||
| configFile: path.join(workspaceRoot, 'babel.config.js'), |
There was a problem hiding this comment.
Switched from cwd to an explicit configFile. cwd did mislead me because I assumed it applies to every relative filepath in the babel config. However, it only applies to the programmatic options of babel-loader:
The working directory that all paths in the programmatic options will be resolved relative to.
|
|
||
| function resolveAliasPath(relativeToBabelConf) { | ||
| const resolvedPath = path.relative(process.cwd(), path.resolve(__dirname, relativeToBabelConf)); | ||
| return `./${resolvedPath.replace('\\', '/')}`; |
There was a problem hiding this comment.
Out of curiosity, why not use an absolute path?
There was a problem hiding this comment.
I would prefer an absolute path as well but I recall this being problematic. You would transpile files to
import Typography from '/home/eps1lon/Development/material-ui/packages/material-ui/src/Typography';And I think bundlers/nextjs would then treat them as external files and not transpile the imported files. Though it's very likely that this was just a symptom of another problem (that might've been fixed).
I can check this in a follow-up (want to see a full,clean CI/CD run) to make sure.
Technically, we should be able to transpile it to import Typography from 'file://home/eps1lon/Development/material-ui/packages/material-ui/src/Typography' i.e. use an actual URI which is valid in ES6 modules. Though this would require a lot of other infra work.
There was a problem hiding this comment.
I think the biggest problem is that this would result in C:\\users\eps1lon\eps1lon\Development\material-ui\packages\material-ui\src\Typography which might be considered a "remote" file (definitely by rsync). This may not be an issue but we get into really weird territory where it's hard to reason about "where" the file is.
I'm not even 100% sure if win32 paths are valid in ES6 modules. So this is all "just works" code that has no solid foundation.
There was a problem hiding this comment.
Your explanation makes a lot of sense. I recall trying this change too and seeing warnings raised by rollup.
There was a problem hiding this comment.
Though definitely right to call this out. This code has now more visibility and the next person who runs into issues with that code is aware that this might just be completely wrong code.
My assumption that
mode: 'production'setsNODE_ENVto'production'inbabel-loaderwas wrong. We didn't actually use the babel aliases at all an instead reliead on webpackresolve.alias.But we do want to run in production.
Test Plan:
26e85fa(#25402)b874ba8(#25402)/cc @mnajdova, @oliviertassinari
Just as a notice in case you made the same assumption as I did.