Skip to content
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

fix build config to work the same when running on windows #11688

Merged
merged 1 commit into from Jul 30, 2020

Conversation

@zxbodya
Copy link
Contributor

zxbodya commented Jun 7, 2020

While debugging failing windows build for #11578 (https://travis-ci.com/github/babel/babel/jobs/345463718) found a problem bundling babel-standalone: it was failing when trying to bundle ./lib/config/files/index.js instead of ./src/config/files/index-browser.ts from babel-core

Fixes for it are in two places:

  • scripts/rollup-plugin-babel-source.js - load there was not handling windows path separator correctly
  • babel.config.js - filter fix in rollup config, some modules failed to compile because babel config overrides were not applied on windows for the same path separator issue.

However, I still do not understand - how current master version can build correctly on windows… It looks like there is some other place in rollup to use browser field, but then - why it would change after converting sources to typescript

Q                       A
Fixed Issues?
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

@babel-bot
Copy link
Collaborator

babel-bot commented Jun 7, 2020

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/25197/

@codesandbox
Copy link

codesandbox bot commented Jun 7, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 35c566f:

Sandbox Source
black-leaf-pywf6 Configuration
sad-herschel-7p7lr Configuration
@@ -115,14 +122,14 @@ module.exports = function(api) {
test: [
"packages/babel-parser",
"packages/babel-helper-validator-identifier",
],
].map(normalize),

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Jun 7, 2020 Member

In Babel 8 I'd like to normalize the paths internally in @babel/core

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Jun 7, 2020

However, I still do not understand - how current master version can build correctly on windows… It looks like there is some other place in rollup to use browser field, but then - why it would change after converting sources to typescript

I admit that I have no idea 🤔

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Jun 7, 2020

https://github.com/zxbodya/babel/blob/997d00ed08f882156bdb0c5217b8f42b2e6babfa/packages/babel-core/package.json#L42

It seems that you didn't change it to .ts in your branch. Maybe it's what is causing the problem?

@zxbodya
Copy link
Contributor Author

zxbodya commented Jun 7, 2020

https://github.com/zxbodya/babel/blob/997d00ed08f882156bdb0c5217b8f42b2e6babfa/packages/babel-core/package.json#L42

It seems that you didn't change it to .ts in your branch. Maybe it's what is causing the problem?

Good catch, indeed changing it to .ts - made it work.

@zxbodya
Copy link
Contributor Author

zxbodya commented Jun 7, 2020

Good catch, indeed changing it to .ts - made it work.

It looks - load in scripts/rollup-plugin-babel-source.js can be removed completely… - I tried building on windows with fixes from PR and without them - both results in exactly the same babel-standalone file.

Also, apparently if not providing custom load - changes in babel.config.js are not really needed(probably somewhere in rollup paths are normalized for files it resolves).

@nicolo-ribaudo what you think? should I revert changes babel.config.js and instead of fixing - to remove load from scripts/rollup-plugin-babel-source.js to avoid confusion because of it?

@JLHwung
Copy link
Contributor

JLHwung commented Jul 2, 2020

However, I still do not understand - how current master version can build correctly on windows…

Currently our Travis Windows CI will skip standalone build, @babel/standalone is only built on Circle CI (Linux).

@zxbodya Can you rebase?

@JLHwung JLHwung changed the base branch from master to main Jul 2, 2020
@zxbodya zxbodya force-pushed the zxbodya:fix-babel-standalone-windows branch from f885813 to 35c566f Jul 2, 2020
@zxbodya
Copy link
Contributor Author

zxbodya commented Jul 2, 2020

@JLHwung rebased it.
however: check my previous comment - might be this change it not really needed

@JLHwung JLHwung merged commit 32e7bb4 into babel:main Jul 30, 2020
8 of 9 checks passed
8 of 9 checks passed
build
Details
test262-pr Workflow: test262-pr
Details
Gitpod Open an online workspace in Gitpod
Details
Travis CI - Pull Request Build Passed
Details
babel/repl REPL preview is available
Details
build-standalone Workflow: build-standalone
Details
ci/codesandbox Building packages succeeded.
Details
codecov/project 91.85% (target 90.00%)
Details
e2e Workflow: e2e
Details
@zxbodya zxbodya deleted the zxbodya:fix-babel-standalone-windows branch Jul 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.