Skip to content

Fix babel-standalone for realz#6137

Merged
Daniel15 merged 3 commits intobabel:7.0from
Daniel15:fix-it-fix-it-fix-it-fix-it
Aug 22, 2017
Merged

Fix babel-standalone for realz#6137
Daniel15 merged 3 commits intobabel:7.0from
Daniel15:fix-it-fix-it-fix-it-fix-it

Conversation

@Daniel15
Copy link
Copy Markdown
Member

@Daniel15 Daniel15 commented Aug 19, 2017

Q                       A
Fixed Issues Closes #6132, Closes #6135, References #6120
Patch: Bug Fix? Yes
Major: Breaking Change? No
Minor: New Feature? No
Tests Added/Pass? N/A / Yes
Spec Compliancy? N/A
License MIT
Doc PR No
Any Dependency Changes?

The root package.json has a reference to babel-loader and babel-core. The root babel-core comes from npm (for some reason) rather than the version in the repo itself, which causes odd issues. By installing babel-loader into babel-standalone's node_modules directory rather than the root node_modules directory, it picks up the correct version of babel-core and everything works fine.

Also made gulpTasks.js properly modular so babel-minify can reuse it.

Huge thanks to @jridgewell for the initial investigation into this issue.

@mention-bot
Copy link
Copy Markdown

mention-bot commented Aug 19, 2017

@Daniel15, thanks for your PR! By analyzing the history of the files in this pull request, we identified @existentialism, @hzoo and @ChauTNguyen to be potential reviewers.

This was referenced Aug 19, 2017
Closed
Closed
@jridgewell
Copy link
Copy Markdown
Member

jridgewell commented Aug 20, 2017

This doesn't seem to fix it. I still get a babel-loader@7.1.1 in the monorepo's node_modules, which causes the same issue.

@Daniel15
Copy link
Copy Markdown
Member Author

Daniel15 commented Aug 20, 2017

Hmm, that's interesting @jridgewell... I wonder why it's working for me locally? I even rebased on top of your PR and it built fine.

I'll keep digging, I might just have to hack the module resolution.

@Daniel15
Copy link
Copy Markdown
Member Author

Daniel15 commented Aug 20, 2017

@jridgewell - Are you sure you ran make bootstrap and make build? I ran make bootstrap which totally blows away node_modules and installs the npm packages, and I do not have babel-loader in the root node_modules.

@jridgewell
Copy link
Copy Markdown
Member

jridgewell commented Aug 20, 2017

Yup.

 ✓ jridgewell@Dandy  ~/src/babel  class-private-properties  git log
Sat Aug 19 20:18:25 2017 -0400 ef6ea1022 (HEAD -> class-private-properties) Merge branch 'pr/6137' into class-private-properties  [Justin Ridgewell]
Sat Aug 19 17:01:15 2017 -0700 eb138bba7 (pr/6137) Fix infinite loop in Makefile (oops)  [Daniel Lo Nigro]
Sat Aug 19 16:41:05 2017 -0700 e5dcf16ab Bump Yarn version to see if it fixes the CircleCI build  [Daniel Lo Nigro]
Sat Aug 19 16:26:34 2017 -0700 16007eccd Fix babel-standalone  [Daniel Lo Nigro]
Sat Aug 19 22:35:40 2017 +0900 c6a094a9d Split export extensions into 2 different plugins, update stage presets (#6080)  [Sangboak Lee]
Fri Aug 18 23:42:39 2017 -0400 3818a64ee (origin/class-private-properties) Final fixes  [Justin Ridgewell]
 ✓ jridgewell@Dandy  ~/src/babel  class-private-properties  node -v
v8.0.0
 ✓ jridgewell@Dandy  ~/src/babel  class-private-properties  npm -v
5.3.0
 ✓ jridgewell@Dandy  ~/src/babel  class-private-properties  yarn --version
0.28.4
 ✓ jridgewell@Dandy  ~/src/babel  class-private-properties  make bootstrap
...
ERROR in ./src/index.js
Module build failed: Error: [BABEL] /Users/jridgewell/src/babel/packages/babel-standalone/src/index.js: You gave us a visitor for the node type "PrivateName" but it's not a valid type
    at verify (/Users/jridgewell/src/babel/node_modules/babel-core/node_modules/babel-traverse/lib/visitors.js:156:13)
    at Function.explode (/Users/jridgewell/src/babel/node_modules/babel-core/node_modules/babel-traverse/lib/visitors.js:59:3)
    at instantiatePlugin (/Users/jridgewell/src/babel/node_modules/babel-core/lib/config/option-manager.js:304:27)
    at loadPluginDescriptor (/Users/jridgewell/src/babel/node_modules/babel-core/lib/config/option-manager.js:280:14)
    at /Users/jridgewell/src/babel/node_modules/babel-core/lib/config/option-manager.js:73:14
    at Array.map (native)
    at OptionManager.mergeOptions (/Users/jridgewell/src/babel/node_modules/babel-core/lib/config/option-manager.js:72:34)
    at /Users/jridgewell/src/babel/node_modules/babel-core/lib/config/option-manager.js:100:15
    at Array.forEach (native)
    at OptionManager.mergeOptions (/Users/jridgewell/src/babel/node_modules/babel-core/lib/config/option-manager.js:99:15)
 ✘ jridgewell@Dandy  ~/src/babel  class-private-properties  ls node_modules | grep babel-loader
babel-loader

@Daniel15
Copy link
Copy Markdown
Member Author

Daniel15 commented Aug 20, 2017 via email

@Daniel15 Daniel15 mentioned this pull request Aug 20, 2017
Merged
@Daniel15 Daniel15 added the WIP label Aug 20, 2017
@Daniel15
Copy link
Copy Markdown
Member Author

Daniel15 commented Aug 20, 2017 •
edited
Loading

@jridgewell Could you please try the latest version and see if that works for you? To test, I rebased a copy of your branch on top of this branch, and it seems to build successfully on CircleCI. There's some test failures, not sure if they're directly related or not (seems not): https://circleci.com/gh/Daniel15/babel/29

jridgewell
jridgewell approved these changes Aug 20, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me, but it feels super hacky. Definitely need someone else to approve.

@Daniel15 Daniel15 removed the WIP label Aug 20, 2017
@Daniel15 Daniel15 force-pushed the fix-it-fix-it-fix-it-fix-it branch from 974ffe4 to 5b9722c Compare August 20, 2017 23:21
@Daniel15
Copy link
Copy Markdown
Member Author

Daniel15 commented Aug 22, 2017

Ping @babel/core - Are you OK with this PR?

@hzoo
Copy link
Copy Markdown
Member

hzoo commented Aug 22, 2017

I guess it feels kinda bad to have to do this but it's broken atm so ok 👍

@hzoo hzoo added the PR: Internal 🏠 A type of pull request used for our changelog categories label Aug 22, 2017
nicolo-ribaudo
nicolo-ribaudo reviewed Aug 22, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would something like this work?

# cwd = babel-core/
yarn link
cd ../babel-standalone/node_modules/babel-loader
yarn link babel-core

It still is very hacky (changing the dependencies of a dependency isn't something you see often), but it feels less hacky than overriding node's module resolution algorithm.

Anyway, this is just internal code: it doesn't need to be best possible.

packages/babel-standalone/src/overrideModuleResolution.js
* override Node's module resolution algorithm to specify a custom resolver for
* babel-core to *force* it to use our version.
*
* Here be dragons.
Copy link
Copy Markdown
Member

@nicolo-ribaudo nicolo-ribaudo Aug 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(You could add an emoji! 🐉 🐲 )

Copy link
Copy Markdown
Member Author

@Daniel15 Daniel15 Aug 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm always worried that inserting Emoji into code is going to break something somewhere 😛

@Daniel15
Copy link
Copy Markdown
Member Author

Daniel15 commented Aug 22, 2017 •
edited
Loading

@nicolo-ribaudo

Would something like this work?

It wouldn't :( The version at babel-standalone/node_packages/babel-core is the correct version, but the version at the root node_modules is the wrong one, and babel-loader is being hoisted to the root. We can likely run yarn link in the root but I was worried that'd break stuff.

@Daniel15 Daniel15 merged commit 93cf26a into babel:7.0 Aug 22, 2017
@Daniel15 Daniel15 deleted the fix-it-fix-it-fix-it-fix-it branch August 22, 2017 20:46
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 6, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@nicolo-ribaudo nicolo-ribaudo nicolo-ribaudo left review comments

@jridgewell jridgewell jridgewell approved these changes

Assignees

No one assigned

Labels

outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Internal 🏠 A type of pull request used for our changelog categories

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Daniel15 @mention-bot @jridgewell @hzoo @nicolo-ribaudo