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

Ignore convert-source-map module in browser builds, fixes @babel/core for browsers #10642

Closed
wants to merge 1 commit into from

Conversation

tajo
Copy link
Contributor

@tajo tajo commented Nov 4, 2019

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

I am using transformFromAstSync function from @babel/core to compile a code in the browser. The issue is that bundler (webpack in my case) eventually reaches the import of convert-source-map module which further imports fs and that results in an error since the fs module is not present in the browser environment.

There a few workarounds / solutions:

  • significantly refactor the @babel/core so convert-source-map is not imported anymore (it's needed only when dealing with sourceMaps)
  • webpack config allows to mock out modules through config.node = { fs: 'empty' } but that adds a lot of friction on the consumer side
  • all popular bundlers understand browser field in the package.json, so we can use it to ignore the convert-source-map import

This PR uses the browser field since it doesn't require any code modifications.

(I was also trying to fork @babel/core in meantime but the problem is that every single preset (plugin) imports @babel/core, so I would have to fork the whole plugin ecosystem which is not feasible.)

@buildsize
Copy link

buildsize bot commented Nov 4, 2019

File name Previous Size New Size Change
babel-preset-env.js 2.77 MB 2.76 MB -5.89 KB (0%)
babel-preset-env.min.js 1.67 MB 1.67 MB -3.48 KB (0%)
babel.js 2.95 MB 2.95 MB -5.11 KB (0%)
babel.min.js 1.63 MB 1.62 MB -3.09 KB (0%)
test262.tap 4.84 MB [deleted]

@nicolo-ribaudo
Copy link
Member

Since convert-source-map often works without the fs module, I don't think that we should just drop its functionality.

I'd like to see this fixed directly in convert-source-map.

This function should be extracted to a separate file:
https://github.com/thlorenz/convert-source-map/blob/ad25a3e4373c641a2241cf1470b81ae123e4c23d/index.js#L28

This file should have a corresponding browser file which exports a function which throws when called.

@tajo
Copy link
Contributor Author

tajo commented Nov 5, 2019

Hm, not sure if this can be fixed only by refactoring convert-source-map. You allow to pass any SourceMap configuration here. So users of @babel/core can pass in something like:

{
  inputSourceMap: {
    isFileComment: true,
    commentFileDir: '...'
  }
}

which would still need to reach fs / path calls.

I guess the ultimate solution would be to browser bundle different normalize-file.js and generate.jsthat would filter out isFileComment and called a modified version of convert-source-map. (Also it might be easier to just inline that library into babel-core (~120LoCs) than asking them to completely reorganize it.)

Anyway, could we still merge this in meantime as a workaround? Afaik, there is no point to bundle convert-source-map into the browser build today since the first import is fs and that always ends up in an exception.

Agreed that this PR doesn't solve the underlying problem but at least if user doesn't care about source maps, @babel/core can be ran in browser.

@nicolo-ribaudo
Copy link
Member

Afaik, there is no point to bundle convert-source-map into the browser build today since the first import is fs and that always ends up in an exception.

@babel/standalone currently has source maps support, and this PR would remove it. It relies on webpack's ability to provide fs mocks.

@tajo
Copy link
Contributor Author

tajo commented Nov 5, 2019

I see. I opened a PR for convert-source-map that adds this into its package.json:

"browser": {
  "fs": false
},

which is the library level version of this mock.

Seems like the most pragmatic way forward.

(Note: it doesn't help when placed into @babel/core's package.json since it's a different module)

@nicolo-ribaudo
Copy link
Member

Thanks!

@JLHwung
Copy link
Contributor

JLHwung commented Nov 5, 2019

Closing this in favour of thlorenz/convert-source-map#68

@tajo Thank you.

@JLHwung JLHwung closed this Nov 5, 2019
@nicolo-ribaudo
Copy link
Member

@tajo Could you open a PR to update convert-source-map in Babel?

@tajo
Copy link
Contributor Author

tajo commented Nov 6, 2019

@nicolo-ribaudo there you go: #10667

@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Feb 5, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Feb 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants