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

Jump to bottom

coffee-script register fails on 1.6.0 & 1.5.1 #8611

Closed
KagamiChan opened this issue Feb 7, 2017 · 8 comments · Fixed by #8618
Closed

coffee-script register fails on 1.6.0 & 1.5.1 #8611

KagamiChan opened this issue Feb 7, 2017 · 8 comments · Fixed by #8618
Assignees
@kevinsawicki

Comments

@KagamiChan
Copy link
Contributor

KagamiChan commented Feb 7, 2017 •
edited

  • Electron version: 1.6.0 / 1.5.1
  • Operating system: Windows 10 x64

Expected behavior

Coffee-script won't fail with require('coffee-script').register() or require('coffee-script/register')

Actual behavior

Coffee-script fails to register

How to reproduce

  1. Download the Electron-quick-start app
  2. install coffee-script via npm i --save coffee-script
  3. Add a register line in index.html, for example,
<!DOCTYPE html>
<html>
  <head>
    <meta charset="UTF-8">
    <title>Hello World!</title>
  </head>
  <body>
    <h1>Hello World!</h1>
    <!-- All of the Node.js APIs are available in this renderer process. -->
    We are using Node.js <script>document.write(process.versions.node)</script>,
    Chromium <script>document.write(process.versions.chrome)</script>,
    and Electron <script>document.write(process.versions.electron)</script>.
  </body>

  <script>
    // You can also require other files to run in this process
    require('coffee-script').register()
    require('./renderer.js')
  </script>
</html>

(require('coffee-script').register() and require('coffee-script/register') will give the same result)

  1. launch the app with Electron 1.5.1 / 1.6.0, error message will appear in Console tab
    image

image

Error message on 1.6.0:

C:\Users\kagami\Documents\git\electron-quick-start\node_modules\coffee-script\lib\coffee-script\coffee-script.js:254 Uncaught TypeError: Cannot read property 'length' of undefined
    at C:\Users\kagami\Documents\git\electron-quick-start\node_modules\coffee-script\lib\coffee-script\coffee-script.js:254:26
    at C:\Users\kagami\Documents\git\electron-quick-start\node_modules\coffee-script\lib\coffee-script\coffee-script.js:425:4
    at Object.<anonymous> (C:\Users\kagami\Documents\git\electron-quick-start\node_modules\coffee-script\lib\coffee-script\coffee-script.js:427:2)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
    at Module.require (module.js:498:17)
    at require (internal/module.js:20:19)

While the same app won't throw any error on 1.5.0 and previous
image

The lines related to this error in coffee-script are:

  if (require.extensions) {
    ref = this.FILE_EXTENSIONS;
    fn1 = function(ext) {
      var base;
      return (base = require.extensions)[ext] != null ? base[ext] : base[ext] = function() {
        throw new Error("Use CoffeeScript.register() or require the coffee-script/register module to require " + ext + " files.");
      };
    };
    for (i = 0, len = ref.length; i < len; i++) {
      ext = ref[i];
      fn1(ext);
    }
  }

this.FILE_EXTENSIONS becomes undefinded but I have no idea how it happens
@gurkerl83
Copy link

Same here!

@MarshallOfSound
Copy link
Member

@KagamiChan Does the coffee script register logic work in normal Node.JS 7.4.0?

@KagamiChan
Copy link
Contributor Author

KagamiChan commented Feb 7, 2017 •
edited

@MarshallOfSound
Yes it works

C:\Users\kagami\Documents\git\electron-quick-start>node
> process.version
'v7.4.0'
> require('coffee-script/register')
{}
> require('coffee-script').register()
{}
>

I've also tested some coffee scripts with register on v7.4.0

@MarshallOfSound
Copy link
Member

MarshallOfSound commented Feb 7, 2017 •
edited

@kevinsawicki This looks like a regression due to electron/node@f691373

Because we now return a function from within the wrapping function the this scope has changed thus this.FILE_EXTENSIONS is not defined. I believe a simple fix would be to change our patch in that commit to be.

(function (exports, require, module, __filename, __dirname, process, global) { 
  return function (exports, require, module, __filename, __dirname) {
    // code
  }.call(this, exports, require, module, __filename, __dirname);
});

Haven't had time to test but I think that should do the trick

@KagamiChan
Copy link
Contributor Author

KagamiChan commented Feb 7, 2017 •
edited

@MarshallOfSound awesome, thank you for looking into it
confirm the patch works by manually modifying the wrapper code for coffee-script.js in the dev tools.

@KagamiChan
Copy link
Contributor Author

Wondering if we could have a spec / test for this kind of packages, if coffee-script is not the only one that stops working ( I'm not sure though)

@kevinsawicki kevinsawicki self-assigned this Feb 7, 2017
@kevinsawicki
Copy link
Contributor

This looks like a regression due to electron/node@f691373

Thanks for digging into this, will work on an update to that patch.

@kevinsawicki kevinsawicki mentioned this issue Feb 7, 2017
Merged
@kevinsawicki
Copy link
Contributor

Wondering if we could have a spec / test for this kind of packages, if coffee-script is not the only one that stops working ( I'm not sure though)

Great suggestion @KagamiChan, spec added in #8618 for coffee-script.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kevinsawicki kevinsawicki

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Forward this in require wrapper electron/electron
4 participants
@gurkerl83 @kevinsawicki @KagamiChan @MarshallOfSound