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

coffeescript is broken on current master #604

Closed
targos opened this issue Sep 22, 2018 · 9 comments
Closed

coffeescript is broken on current master #604

targos opened this issue Sep 22, 2018 · 9 comments

Comments

@targos
Copy link
Member

targos commented Sep 22, 2018

It seems serious, because related to the module loader:

/cc @jashkenas @GeoffreyBooth

$ citgm coffeescript
info:    starting            | coffeescript
info:    lookup              | coffeescript
info:    lookup-found        | coffeescript
info:    coffeescript lookup-replace| https://github.com/jashkenas/coffeescript/archive/6e86b67818405cc2210d24ad826537836758c76a.tar.gz
info:    coffeescript npm:   | Downloading project: https://github.com/jashkenas/coffeescript/archive/6e86b67818405cc2210d24ad826537836758c76a.tar.gz
info:    coffeescript npm:   | Project downloaded coffeescript-2.3.2.tgz
info:    coffeescript npm:   | npm install started
warn:    coffeescript npm-install:| npm WARN You are using a pre-release version of node and things may not work as expected
warn:    coffeescript npm-install:| npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@1.2.4 (node_modules/fsevents):                                  
warn:                             | npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@1.2.4: wanted {"os":"darwin","arch":"any"} (curren
info:    coffeescript npm:   | npm install successfully completed
info:    coffeescript npm:   | test suite started
error:   failure             | The canary is dead:
error:   failing module(s)   |
error:   module name:        | coffeescript
error:   version:            | 2.3.2
error:   error:              | The canary is dead:
error:   error:              | undefinedadded 505 packages from 1302 contributors and audited 4832 packages in 4.264s                                        
error:                       | found 0 vulnerabilities                                                                                                       
error:                       |                                                                                                                               
error:                       |                                                                                                                               
error:                       | > coffeescript@2.3.2 test /tmp/7a69335a-85f6-44ef-8429-d1d1e66f4023/coffeescript                                              
error:                       | > node ./bin/cake test                                                                                                        
error:                       |                                                                                                                               
error:                       | failed 2 and passed 1274 tests in 4.05 seconds
error:                       |                                                                                                                               
error:                       |   literate coffeescript modules can be imported
error:                       |   Error: Cannot find module './importing/'                                                                                    
error:                       | at Function.Module._resolveFilename (internal/modules/cjs/loader.js:595:15)                                                   
error:                       | at Function.Module._load (internal/modules/cjs/loader.js:521:25)                                                              
error:                       | at Module.require (internal/modules/cjs/loader.js:651:17)                                                                     
error:                       | at require (internal/modules/cjs/helpers.js:20:18)                                                                            
error:                       | at Function.<anonymous> (/tmp/7a69335a-85f6-44ef-8429-d1d1e66f4023/coffeescript/test/importing.coffee:34:7)                   
error:                       | at global.test (/tmp/7a69335a-85f6-44ef-8429-d1d1e66f4023/coffeescript/Cakefile:538:21)                                       
error:                       | at Object.<anonymous> (/tmp/7a69335a-85f6-44ef-8429-d1d1e66f4023/coffeescript/test/importing.coffee:30:3)                     
error:                       | at Object.<anonymous> (/tmp/7a69335a-85f6-44ef-8429-d1d1e66f4023/coffeescript/test/importing.coffee:1:1)                      
error:                       | at Module._compile (internal/modules/cjs/loader.js:703:30)                                                                    
error:                       | at Object.loadFile (/tmp/7a69335a-85f6-44ef-8429-d1d1e66f4023/coffeescript/lib/coffeescript/register.js:18:19)                
error:                       | at Module.load (/tmp/7a69335a-85f6-44ef-8429-d1d1e66f4023/coffeescript/lib/coffeescript/register.js:53:36)                    
error:                       | at tryModuleLoad (internal/modules/cjs/loader.js:552:12)                                                                      
error:                       | at Function.Module._load (internal/modules/cjs/loader.js:544:3)                                                               
error:                       | at Module.require (internal/modules/cjs/loader.js:651:17)                                                                     
error:                       | at require (internal/modules/cjs/helpers.js:20:18)                                                                            
error:                       | at Function.<anonymous> (/tmp/7a69335a-85f6-44ef-8429-d1d1e66f4023/coffeescript/test/importing.coffee:17:12)                  
error:                       | at global.test (/tmp/7a69335a-85f6-44ef-8429-d1d1e66f4023/coffeescript/Cakefile:538:21)                                       
error:                       | at Object.<anonymous> (/tmp/7a69335a-85f6-44ef-8429-d1d1e66f4023/coffeescript/test/importing.coffee:5:3)                      
error:                       | at Object.<anonymous> (/tmp/7a69335a-85f6-44ef-8429-d1d1e66f4023/coffeescript/test/importing.coffee:1:1)                      
error:                       | at Module._compile (internal/modules/cjs/loader.js:703:30)                                                                    
error:                       | at Object.CoffeeScript.run (/tmp/7a69335a-85f6-44ef-8429-d1d1e66f4023/coffeescript/lib/coffeescript/index.js:67:23)           
error:                       | at runTests (/tmp/7a69335a-85f6-44ef-8429-d1d1e66f4023/coffeescript/Cakefile:609:22)                                          
error:                       | at Object.action (/tmp/7a69335a-85f6-44ef-8429-d1d1e66f4023/coffeescript/Cakefile:623:12)                                     
error:                       | at invoke (/tmp/7a69335a-85f6-44ef-8429-d1d1e66f4023/coffeescript/lib/coffeescript/cake.js:57:26)                             
error:                       | at Object.exports.run (/tmp/7a69335a-85f6-44ef-8429-d1d1e66f4023/coffeescript/lib/coffeescript/cake.js:87:20)                 
error:                       | at Object.<anonymous> (/tmp/7a69335a-85f6-44ef-8429-d1d1e66f4023/coffeescript/bin/cake:22:42)                                 
error:                       | at Module._compile (internal/modules/cjs/loader.js:703:30)                                                                    
error:                       | at Object.Module._extensions..js (internal/modules/cjs/loader.js:714:10)                                                      
error:                       | at Module.load (internal/modules/cjs/loader.js:613:32)                                                                        
error:                       | at tryModuleLoad (internal/modules/cjs/loader.js:552:12)                                                                      
error:                       | at Function.Module._load (internal/modules/cjs/loader.js:544:3)                                                               
error:                       | at Function.Module.runMain (internal/modules/cjs/loader.js:756:12)                                                            
error:                       | at startup (internal/bootstrap/node.js:302:19)                                                                                
error:                       | at bootstrapNodeJSCore (internal/bootstrap/node.js:855:3)                                                                     
error:                       |                                                                                                                               
error:                       | function() {                                                                                                                  
error:                       | var base, i, len, magicVal, module, ref, results;                                                                             
error:                       | magicVal = 3;                                                                                                                 
error:                       | ref = ' .import.coffee.md import.coffee.md import.litcoffee import.extension.coffee.md'.split(' ');                           
error:                       | // Leading space intentional to check for index.coffee.md                                                                     
error:                       | results = [];                                                                                                                 
error:                       | for (i = 0, len = ref.length; i < len; i++) {                                                                                 
error:                       | module = ref[i];                                                                                                              
error:                       | results.push(ok((typeof (base = require(`./importing/${module}`)).value === "function" ? base.value() : void 0) === magicVal, module));
error:                       | }                                                                                                                             
error:                       | return results;                                                                                                               
error:                       | }                                                                                                                             
error:                       |                                                                                                                               
error:                       |   literate coffeescript modules can be imported
error:                       |   Error: Cannot find module './importing/'                                                                                    
error:                       | at Function.Module._resolveFilename (internal/modules/cjs/loader.js:595:15)                                                   
error:                       | at Function.Module._load (internal/modules/cjs/loader.js:521:25)                                                              
error:                       | at Module.require (internal/modules/cjs/loader.js:651:17)                                                                     
error:                       | at require (internal/modules/cjs/helpers.js:20:18)                                                                            
error:                       | at Function.<anonymous> (/tmp/7a69335a-85f6-44ef-8429-d1d1e66f4023/coffeescript/test/importing.coffee:34:7)                   
error:                       | at global.test (/tmp/7a69335a-85f6-44ef-8429-d1d1e66f4023/coffeescript/Cakefile:538:21)                                       
error:                       | at Object.<anonymous> (/tmp/7a69335a-85f6-44ef-8429-d1d1e66f4023/coffeescript/test/importing.coffee:30:3)                     
error:                       | at Object.<anonymous> (/tmp/7a69335a-85f6-44ef-8429-d1d1e66f4023/coffeescript/test/importing.coffee:1:1)                      
error:                       | at Module._compile (internal/modules/cjs/loader.js:703:30)                                                                    
error:                       | at Object.CoffeeScript.run (/tmp/7a69335a-85f6-44ef-8429-d1d1e66f4023/coffeescript/lib/coffeescript/index.js:67:23)           
error:                       | at runTests (/tmp/7a69335a-85f6-44ef-8429-d1d1e66f4023/coffeescript/Cakefile:609:22)                                          
error:                       | at Object.action (/tmp/7a69335a-85f6-44ef-8429-d1d1e66f4023/coffeescript/Cakefile:623:12)                                     
error:                       | at invoke (/tmp/7a69335a-85f6-44ef-8429-d1d1e66f4023/coffeescript/lib/coffeescript/cake.js:57:26)                             
error:                       | at Object.exports.run (/tmp/7a69335a-85f6-44ef-8429-d1d1e66f4023/coffeescript/lib/coffeescript/cake.js:87:20)                 
error:                       | at Object.<anonymous> (/tmp/7a69335a-85f6-44ef-8429-d1d1e66f4023/coffeescript/bin/cake:22:42)                                 
error:                       | at Module._compile (internal/modules/cjs/loader.js:703:30)                                                                    
error:                       | at Object.Module._extensions..js (internal/modules/cjs/loader.js:714:10)                                                      
error:                       | at Module.load (internal/modules/cjs/loader.js:613:32)                                                                        
error:                       | at tryModuleLoad (internal/modules/cjs/loader.js:552:12)                                                                      
error:                       | at Function.Module._load (internal/modules/cjs/loader.js:544:3)                                                               
error:                       | at Function.Module.runMain (internal/modules/cjs/loader.js:756:12)                                                            
error:                       | at startup (internal/bootstrap/node.js:302:19)                                                                                
error:                       | at bootstrapNodeJSCore (internal/bootstrap/node.js:855:3)                                                                     
error:                       |                                                                                                                               
error:                       | function() {                                                                                                                  
error:                       | var base, i, len, magicVal, module, ref, results;                                                                             
error:                       | magicVal = 3;                                                                                                                 
error:                       | ref = ' .import.coffee.md import.coffee.md import.litcoffee import.extension.coffee.md'.split(' ');                           
error:                       | // Leading space intentional to check for index.coffee.md                                                                     
error:                       | results = [];                                                                                                                 
error:                       | for (i = 0, len = ref.length; i < len; i++) {                                                                                 
error:                       | module = ref[i];                                                                                                              
error:                       | results.push(ok((typeof (base = require(`./importing/${module}`)).value === "function" ? base.value() : void 0) === magicVal, module));
error:                       | }                                                                                                                             
error:                       | return results;                                                                                                               
error:                       | }                                                                                                                             
error:                       |                                                                                                                               
error:                       | npm WARN You are using a pre-release version of node and things may not work as expected                                      
error:                       | npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@1.2.4 (node_modules/fsevents):                                       
error:                       | npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@1.2.4: wanted {"os":"darwin","arch":"any"} (current: {"
error:                       |                                                                                                                               
error:                       | npm ERR! Test failed.  See above for more details.                                                                            
error:   done                | The smoke test has failed.
info:    duration            | test duration: 13836ms

Tested with this nightly build: https://nodejs.org/download/nightly/v11.0.0-nightly201809222b29df71eb/
@GeoffreyBooth
Copy link
Member

The two tests that are failing have to do with CoffeeScript registering to handle requires for files with a .coffee.md extension. That code hasn’t changed on CoffeeScript’s side in many years, so I assume it’s something that recently changed in Node 11 that’s causing these errors to appear. These “double” extensions aren’t terribly common, but they’re not unique to CoffeeScript; I’m sure there are other projects that use extensions like .es6.js and the like.

@targos
Copy link
Member Author

targos commented Sep 23, 2018

Thanks @GeoffreyBooth. The behaviour changed in nodejs/node#22382.
However, it looks like it was already broken before? See nodejs/node#4778.

I don't know exactly how CoffeeScript configures the module loader, but with this simple test, I can't make it work (in Node 10):

echo "console.log('in a.coffee')" > a.coffee
echo "console.log('in b.coffee.md')" > b.coffee.md
// test.js
require.extensions['.coffee'] = () => console.log('.coffee');
require.extensions['.coffee.md'] = () => console.log('.coffee.md');

require('./a')
require('./b')
$ ./node test.js
.coffee /home/mzasso/git/nodejs/node/a.coffee
in b.coffee.md

We can see that the .coffee extension is correctly loaded with our custom loader (which just logs).
The .coffee.md is resolved, but the custom loader isn't used. Instead, it falls back to the .js loader.

@targos
Copy link
Member Author

targos commented Sep 23, 2018

/cc @lundibundi @mscdex @BridgeAR @jdalton @addaleax @jasnell who were involved in the pull request.

@GeoffreyBooth
Copy link
Member

CoffeeScript does some hacking of module to make it possible:

https://github.com/jashkenas/coffeescript/blob/master/src/register.coffee#L12-L37 (generated JS)

This code was last edited in 2013. Obviously from looking at it whoever wrote it shouldn’t have expected it to last forever, considering the overriding of Node’s prototypes and using of Node’s private methods that it’s doing . . . but does it need to break just yet? I’d love to leave all the CommonJS code in here alone and only worry about refactoring for ESM when that day comes.

@targos Your test needs a few tweaks. First, by definition Literate CoffeeScript (the .coffee.md file) needs to be indented; anything at column 0 is interpreted as a comment. Also, you need to call coffeescript/register to load its module loader. I also rewrote the tests so that they use console.log without parentheses, so that the code will throw an exception if it’s not first transpiled by CoffeeScript. Here’s a revised test:

echo "console.log 'in a.coffee'" > a.coffee
echo "    console.log 'in b.coffee.md'" > b.coffee.md
echo "require('coffeescript/register'); require('./a'); require('./b');" > test.js
npm install coffeescript

And running it:

$ node -v
v10.11.0
$ node test.js
in a.coffee
in b.coffee.md

@jdalton
Copy link
Member

jdalton commented Sep 23, 2018 •
edited
Loading

The nodejs/node#22382 filters Module._extensions keys by those that are a trailing extension.
So if the keys were ['.coffee.md', '.js'] the filtered result would be ['.js'].

Under the nodejs/node#22382 change for .md to show up in the filtered result there would need to be a compiler set for it Module._extensions[".md"] = ....

The legacy behavior allowed Node to find the path of foo.coffee.md when looking for foo, by adding a Module._extensions entry for .coffee.md, but would not be able to compile the found file because Node uses path.extname() on filenames to resolve the Module._extensions compiler in Module#load(). So CoffeeScript worked around that limitation with their findExtension helper for their Module._load() overwrite.

For CoffeeScript, the way to work with nodejs/node#22382 would be to register a .md compiler for Module._extensions that would check the filename for .coffee.md and handle it if found or defer to the .js extension compiler.

The other option is for Node to revert nodejs/node#22382 and flip to supporting multi-part extensions in Module#load(). This way files with .coffee.md will be found (as with the legacy behavior) but they will also be compilable in Module#load(). This should also play well with the ecosystem since projects like CoffeeScript are fixing the issue with Module#load so won't be affected by a now-working-version of Module#load() and others coming fresh to this won't need to monkey patch Node to get it working.

@MylesBorins
Copy link
Contributor

@jasnell @addaleax as the TSC members who approved this Semver-Major change do you have an opinion?

@addaleax
Copy link
Member

@MylesBorins I don’t have a strong preference, but if we do decide to do something in Node.js core, I like the last option proposed by @jdalton (reverting + making multi-extension loading work).

@BridgeAR
Copy link
Member

BridgeAR commented Sep 24, 2018 •
edited
Loading

I am on the same page as @addaleax

@jdalton
Copy link
Member

jdalton commented Sep 25, 2018 •
edited
Loading

Ok. In the short term I'll try to get a PR out today or tomorrow to revert nodejs/node#22382 (if someone wants to beat me to it that's cool too).

@GeoffreyBooth would you like to pair program with me this week to create a PR to make Module#load work with multi-part extensions?

@jdalton jdalton mentioned this issue Oct 2, 2018
Closed
1 task
danbev pushed a commit to nodejs/node that referenced this issue Oct 10, 2018
@jdalton @danbev
Revert "module: fix inconsistency between load and _findPath"
b6dcf8c 
This reverts commit 1b92214
from PR #22382.

See the discussion at
nodejs/citgm#604

PR-URL: #23228
Refs: #22382
Fixes: #4778
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@GeoffreyBooth GeoffreyBooth mentioned this issue Oct 11, 2018
Closed
4 tasks
jasnell pushed a commit to nodejs/node that referenced this issue Oct 17, 2018
@jdalton @jasnell
Revert "module: fix inconsistency between load and _findPath"
15b1241 
This reverts commit 1b92214
from PR #22382.

See the discussion at
nodejs/citgm#604

PR-URL: #23228
Refs: #22382
Fixes: #4778
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@targos targos closed this as completed Dec 15, 2018
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
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Revert "module: fix inconsistency between load and _findPath"
6 participants
@jdalton @GeoffreyBooth @MylesBorins @addaleax @targos @BridgeAR