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.

Sign up for GitHub

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

Jump to bottom

Upstream Problem with process polyfill #3

Closed
CMCDragonkai opened this issue May 14, 2018 · 8 comments
Closed

Upstream Problem with process polyfill #3

CMCDragonkai opened this issue May 14, 2018 · 8 comments

Comments

@CMCDragonkai
Copy link
Member

The process polyfill hasn't added the platform property which makes not only your own code fail but also other libraries fail that expect there to be a platform property.

defunctzombie/node-process#55

Then test with:

<html>
  <head>
    <meta charset="UTF-8">
    <title>test</title>
    <script type="text/javascript" src="./dist/index.browser.umd.js"></script>
  </head>
  <body>
    <p>test</p>
  </body>
</html>
@CMCDragonkai CMCDragonkai mentioned this issue May 15, 2018
Merged
@CMCDragonkai
Copy link
Member Author

CMCDragonkai commented Jun 3, 2018 •
edited
Loading

While the platform property doesn't exist. The browser error is actually coming from the lack of process itself. The process shim is being loaded as a browser object. And the check for platform is only checking if the platform equals win32. Which in the shim currently would be undefined or if later the PR gets merged it would be browser. The actual problem comes from another library that uses process but the process object doesn't exist in the outputted UMD object.

The library using process implicitly (without requiring it) is the path shim.

@CMCDragonkai
Copy link
Member Author

If rollup were rolling up the process shim as process instead of browser it would work. But I don't know why rollup is rolling up the shim as browser instead.

@CMCDragonkai
Copy link
Member Author

CMCDragonkai commented Jun 3, 2018 •
edited
Loading

It could be due https://github.com/defunctzombie/node-process/blob/master/package.json#L18

Which we are using due to the resolve setting to browser: true.

Which can also be happening because the path library doesn't do:

var process = require('process');

@CMCDragonkai
Copy link
Member Author

I just tested it. It all works if the path library were to be explicitly require('process') before hand. But because they just rely on the global process that's when it all fails and rollup isn't able to detect it. Even if I were to ask them to do that, it would still fail on other libraries that rely on the global process. The only fullproof way is to inject a global into the module called process to rely on the shim.

@CMCDragonkai
Copy link
Member Author

I need to find a way to inject globals into the UMD bundle. Otherwise things like process and Buffer are just not being found, because the dependencies just use the builtins/globals that exist in the current context.

@CMCDragonkai
Copy link
Member Author

rollup/rollup-plugin-node-resolve#163

@CMCDragonkai
Copy link
Member Author

The only solution is to throw away all the current polyfills in devDependencies and solely rely on rollup-plugin-node-globals and rollup-plugin-node-builtins. The first brings in globals, the second allows those globals to be required/imported. I tested it and it fixes these errors. However I'm not able to test the full functionality because of the error with readable-stream.

I'm not sure if this works well, since I am relying on good Buffer behaviour most importantly. The best way is to make the ava tests run in the browser (or run the ava tests against the built distributions instead of the source).

@CMCDragonkai
Copy link
Member Author

CMCDragonkai commented Jun 3, 2018 •
edited
Loading

This is the UMD target now:

  {
    input: 'lib/index.js',
    output: {
      file: 'dist/index.browser.umd.js',
      format: 'umd',
      name: 'virtualfs'
    },
    plugins: [
      babel({
        babelrc: false,
        exclude: 'node_modules/**',
        runtimeHelpers: true,
        plugins: ['transform-object-rest-spread', 'transform-runtime', 'transform-class-properties'],
        presets: [
          'flow',
          ['env', {
            modules: false,
            targets: {
              browsers: ['last 2 versions']
            }
          }]
        ]
      }),
      resolve({
        preferBuiltins: true,
        browser: true
      }),
      commonjs({
        namedExports: {
          'node_modules/process/browser.js': ['nextTick']
        }
      }),
      globals(),
      builtins()
    ]
  }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@CMCDragonkai