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

Update virtualfs dependency #54

Merged
merged 1 commit into from
Jun 4, 2018
Merged

Update virtualfs dependency #54

merged 1 commit into from
Jun 4, 2018

Conversation

mathiasbynens
Copy link
Member

@mathiasbynens mathiasbynens commented May 13, 2018

@CMCDragonkai When updating to [email protected] as suggested in #53, the generated benchmark fails to run:

$ npm run benchmark

> [email protected] benchmark ~/projects/web-tooling-benchmark
> node dist/cli.js

~/projects/web-tooling-benchmark/dist/cli.js:175687
    Error.captureStackTrace(this, arguments.callee);
                                            ^

TypeError: 'caller', 'callee', and 'arguments' properties may not be accessed on strict mode functions or the arguments objects for calls to them
    at new CustomError (~/projects/web-tooling-benchmark/dist/cli.js:175687:45)
    at createError (~/projects/web-tooling-benchmark/dist/cli.js:175711:43)
    at ce (~/projects/web-tooling-benchmark/dist/cli.js:175717:12)
    at custom (~/projects/web-tooling-benchmark/dist/cli.js:175721:25)
    at ~/projects/web-tooling-benchmark/dist/cli.js:176038:25
    at createCommonjsModule (~/projects/web-tooling-benchmark/dist/cli.js:166043:35)
    at ~/projects/web-tooling-benchmark/dist/cli.js:175726:13
    at commonjsGlobal (~/projects/web-tooling-benchmark/dist/cli.js:166029:10)
    at Object.<anonymous> (~/projects/web-tooling-benchmark/dist/cli.js:166032:2)
    at Object.exports.byteLength (~/projects/web-tooling-benchmark/dist/cli.js:182197:30)

The offending line is part of virtualfs/dist/index.browser.umd.js. Would you be open to changing this to add strict mode support?

@CMCDragonkai
Copy link

Oh that's embarassing. I'll check it tonight.

@CMCDragonkai
Copy link

CMCDragonkai commented May 14, 2018

My tests show that this is occurring because moving from 1.x to 2.x, I added:

  "main": "dist/index.node.cjs.js",
  "module": "dist/index.node.es.js",
  "browser": "dist/index.browser.umd.js",

To the package.json in order to split the output for Node CommonJS, ES6 Module and Web Browser bundlers.

It appears that your current webpack builder is building using the browser target.

If I run npm test on your repo, it passes all tests with the 2.0.5 js-virtualfs package.

If I try to do const virtualfs = require('./dist/index.browser.umd.js') in Node on js-virtualfs it fails with this error:

TypeError: 'caller', 'callee', and 'arguments' properties may not be accessed on strict mode functions or the arguments objects for calls to them
    at new CustomError (/home/cmcdragonkai/Projects/js-virtualfs/dist/index.browser.umd.js:9691:45)
    at createError (/home/cmcdragonkai/Projects/js-virtualfs/dist/index.browser.umd.js:9715:43)
    at ce (/home/cmcdragonkai/Projects/js-virtualfs/dist/index.browser.umd.js:9721:12)
    at custom (/home/cmcdragonkai/Projects/js-virtualfs/dist/index.browser.umd.js:9725:25)
    at /home/cmcdragonkai/Projects/js-virtualfs/dist/index.browser.umd.js:10042:25
    at createCommonjsModule (/home/cmcdragonkai/Projects/js-virtualfs/dist/index.browser.umd.js:16:35)
    at /home/cmcdragonkai/Projects/js-virtualfs/dist/index.browser.umd.js:9730:13
    at commonjsGlobal (/home/cmcdragonkai/Projects/js-virtualfs/dist/index.browser.umd.js:2:65)
    at Object.<anonymous> (/home/cmcdragonkai/Projects/js-virtualfs/dist/index.browser.umd.js:5:2)
    at Module._compile (module.js:624:30)

Which isn't an error of js-virtualfs, but an error within babel or rollup.

At any case while I investigate why rollup/babel is creating a bad UMD bundle of the code, I think the webpack bundler you have shouldn't be using the browser export target. It should be using the main cjs target or module target since webpack should recognise ES6 modules.

@CMCDragonkai
Copy link

CMCDragonkai commented May 14, 2018

Aha. I think I found the source of the error, in my upstream dependency: rvagg/node-errno#17. And also this calvinmetcalf/rollup-plugin-node-builtins#31 from rollup.

I updated errno which fixes the strictness issue. However the rollup issue is still a problem. However there is actually another problem with the browser target. The node-process polyfill for process doesn't fully specify the platform property, this PR still hasn't been merged defunctzombie/node-process#82.

The best solution right now is to make sure that webpack isn't using the browser target. The benchmark is meant to be used with nodejs anyway.

@mathiasbynens
Copy link
Member Author

Thanks for digging into this, @CMCDragonkai!

The best solution right now is to make sure that webpack isn't using the browser target. The benchmark is meant to be used with nodejs anyway.

The Web Tooling Benchmark is kind of a special case, in that the generated benchmark should also run in browsers.

@CMCDragonkai
Copy link

These 2 issues are tracking upstream issues before this problem can be fixed:

I just reread the README, and I can see that the benchmark can be used from the browser as well.

My original thinking for downstream users who were bundling for browsers as well, are still supposed to use the main/module targets and build in the polyfills at their bundling stage, rather than relying on the upstream browser target which already embeds polyfills that may not be suitable for downstream environment (due to code duplication...).

@CMCDragonkai
Copy link

CMCDragonkai commented Jun 3, 2018

I've released 2.1.0 and I have ran the npm run benchmark and there are no more errors.

[nix-shell:~/Projects/web-tooling-benchmark]$ npm run benchmark

> [email protected] benchmark /home/cmcdragonkai/Projects/web-tooling-benchmark
> node dist/cli.js

Running Web Tooling Benchmark 0.4.0...
--------------------------------------
         acorn:  6.44 runs/sec
         babel:  7.51 runs/sec
       babylon:  7.12 runs/sec
         buble:  5.36 runs/sec
          chai: 11.38 runs/sec                                                     
  coffeescript:  5.94 runs/sec                                                     
        espree:  1.54 runs/sec                                                     
       esprima:  7.91 runs/sec                                                     
        jshint:  6.87 runs/sec                                                     
         lebab:  8.54 runs/sec                                                     
       postcss:  5.33 runs/sec                                                     
       prepack:  6.62 runs/sec                                                     
      prettier:  5.33 runs/sec
    source-map:  8.37 runs/sec
    typescript:  8.93 runs/sec
     uglify-es: 15.30 runs/sec
     uglify-js:  4.42 runs/sec
--------------------------------------
Geometric mean:  6.60 runs/sec

I had to swap all the polyfills to rollup plugin based rather than explicit dev dependencies.

@mathiasbynens
Copy link
Member Author

@CMCDragonkai Thanks for your work on this!

@mathiasbynens mathiasbynens changed the title [WIP] Update virtualfs dependency [DO NOT MERGE] Update virtualfs dependency Jun 4, 2018
@mathiasbynens mathiasbynens merged commit 0f098f5 into master Jun 4, 2018
@mathiasbynens mathiasbynens deleted the update-virtualfs branch June 4, 2018 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants