Fix #4703, 4713: Transpile fixes #4717

GeoffreyBooth · 2017-09-23T20:21:32Z

This PR finishes the “remove Babel as a dependency” work (#4703), and gets us out of the business of finding Babel’s options for it (#4713). The latter makes us more compatible with additional options, um, options, that Babel is introducing in Babel 7; and lessens our integration with Babel if we want to support additional transpilers in the future. The docs are also updated: http://rawgit.com/GeoffreyBooth/coffeescript/transpile-fixes/docs/v2/index.html

I would appreciate help testing this, as there aren’t tests for running this via the command line. I’m also on a Mac, so anyone on Windows or Linux, your feedback would be especially appreciated. cc @loganfsmyth

I think once this is merged in we should release a 2.0.1, that includes this and #4668 / #4712.

@jashkenas, are you okay with the docs changes?

lydell · 2017-09-24T07:50:29Z

I'm confused. I tried to trigger the "you don't have babel installed" error, but failed.

~/forks/coffeescript transpile-fixes
❯ git log -1
commit 3528d8f1a0b035f905ee76cce83a08d2f3f59106 (HEAD -> transpile-fixes, geoffrey/transpile-fixes)
Author: Geoffrey Booth <[email protected]>
Date:   2017-09-23 13:16:56 -0700

    Docs cleanup

~/forks/coffeescript transpile-fixes  
❯ cd ..

~/forks  
❯ mkdir test

~/forks  
❯ cd test

~/forks/test  
❯ npm uninstall -g babel-core
up to date in 0.046s

~/forks/test  
❯ look-up package.json .babelrc .babelrc.js

~/forks/test  
❯ node -e 'require("babel-core")'
module.js:491
    throw err;
    ^

Error: Cannot find module 'babel-core'
    at Function.Module._resolveFilename (module.js:489:15)
    at Function.Module._load (module.js:439:25)
    at Module.require (module.js:517:17)
    at require (internal/module.js:11:18)
    at [eval]:1:1
    at ContextifyScript.Script.runInThisContext (vm.js:44:33)
    at Object.runInThisContext (vm.js:116:38)
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (module.js:573:30)
    at evalScript (bootstrap_node.js:435:27)

~/forks/test  
❯ echo test > test.coffee

~/forks/test  
❯ ../coffeescript/bin/coffee --transpile --compile test.coffee

~/forks/test  
❯ cat test.js
// Generated by CoffeeScript 2.0.0
(function () {
  test;
}).call(this);⏎

Isn't the above supposed to have triggered the error? I'm on Ubuntu 16.04.

GeoffreyBooth · 2017-09-24T18:08:10Z

@lydell I can’t explain what you’re seeing. I tried a controlled test using Docker. I created this Dockerfile:

FROM node:latest

COPY ./entrypoint.sh /entrypoint.sh

RUN chmod +x /entrypoint.sh

ENTRYPOINT /entrypoint.sh

with this entrypoint.sh:

#!/bin/bash

mkdir ~/project && cd ~/project

echo 'import path from "path"; echo path.sep' > ./test.coffee

npm install --global \
  https://github.com/GeoffreyBooth/coffeescript.git#transpile-fixes

coffee --transpile test.coffee || true

npm uninstall --global coffeescript

echo '{ "private": true }' > ./package.json

echo '{}' > ./.babelrc

npm install --save-dev \
  https://github.com/GeoffreyBooth/coffeescript.git#transpile-fixes

./node_modules/.bin/coffee --transpile test.coffee || true

And ran docker build . -t test and then docker run test, and the latter produced this output:

npm info it worked if it ends with ok
npm info using [email protected]
npm info using [email protected]
npm info lifecycle [email protected]~prepack: [email protected]
npm info lifecycle [email protected]~postpack: [email protected]
npm info lifecycle [email protected]~preinstall: [email protected]
npm info linkStuff [email protected]
/usr/local/bin/coffee -> /usr/local/lib/node_modules/coffeescript/bin/coffee
/usr/local/bin/cake -> /usr/local/lib/node_modules/coffeescript/bin/cake
npm info lifecycle [email protected]~install: [email protected]
npm info lifecycle [email protected]~postinstall: [email protected]
+ [email protected]
added 1 package in 2.178s
npm info ok
To use --transpile with globally-installed CoffeeScript, you must have babel-core 
installed globally:
  npm install --global babel-core
And you must save options to configure Babel in one of the places it looks to
find its options, relative to the file being compiled or to the current folder.
See http://coffeescript.org/#transpilation
npm info it worked if it ends with ok
npm info using [email protected]
npm info using [email protected]
npm info lifecycle [email protected]~preuninstall: [email protected]
npm info lifecycle [email protected]~uninstall: [email protected]
npm info lifecycle [email protected]~postuninstall: [email protected]
removed 1 package in 0.062s
npm info ok
npm info it worked if it ends with ok
npm info using [email protected]
npm info using [email protected]
npm info lifecycle [email protected]~preinstall: [email protected]
npm info linkStuff [email protected]
npm info lifecycle [email protected]~install: [email protected]
npm info lifecycle [email protected]~postinstall: [email protected]
npm info lifecycle undefined~preshrinkwrap: undefined
npm info lifecycle undefined~shrinkwrap: undefined
npm notice created a lockfile as package-lock.json. You should commit this file.
npm info lifecycle undefined~postshrinkwrap: undefined
+ [email protected]
added 1 package in 0.52s
npm info ok
To use --transpile, you must have babel-core installed:
  npm install --save-dev babel-core
And you must save options to configure Babel in one of the places it looks to
find its options.
See http://coffeescript.org/#transpilation

GeoffreyBooth · 2017-09-24T18:11:32Z

@lydell Wait, looking at your code again, it appears you’re running bin/coffee from a checkout of the repo? That could explain what you’re seeing, since the repo has a devDependency of babel-core and therefore babel-core is in the node_modules folder of the repo.

What if you try npm installing from this branch like I did? Or npm install --no-optional coffeescript (the released 2.0.0) and then manually replace the lib folder inside /usr/local/lib/node_modules/coffeescript/ or ./node_modules/coffeescript with the lib folder from this branch?

…e CLI and babel-core isn’t installed

…el search for its options, rather than us doing so

lydell · 2017-09-24T18:32:17Z

Yep, when installing like you suggest I get the error message as expected.

Isn’t that strange though? How can a babel-core installation in the node_modules/ of the coffeescript installation affect things?

GeoffreyBooth · 2017-09-24T20:50:37Z

Yeah, but out of our responsibility I think. The mysteries of Node require.

loganfsmyth · 2017-09-25T01:28:33Z

lib/coffeescript/command.js

+        } else {
+          opts.transpile.filename = base || process.cwd();
+          if (opts.transpile.filename.endsWith(path.sep)) {
+            opts.transpile.filename += '.';


For the sake of making the error messages clearer, I wonder if it's be better to have this do <stdio>.coffee <fake>.coffee or something? Not sure what cases come up where the filename isn't known.

I guess relatedly, should Babel use the .babelrc if it doesn't know a filename? Babel's CLI itself for instance does not take .babelrc files when code is passed in via stdin, though we've talked about adding an opt-in flag for that behavior. I don't have strong feelings how you do it though, since it's probably fine as long as it's documented for users.

The only cases I can think of where filename would be undefined are when the coffee command is called with input via stdin or via --eval, e.g.:

./bin/coffee --transpile --eval 'import path from "path" console.log path.sep throw new Error()' / Error at Object.<anonymous> (<anonymous>:1:54) at Module._compile (module.js:573:30) at Object.CoffeeScript.run (~/Sites/coffeescript/lib/coffeescript/index.js:61:23)

And the CoffeeScript compiler is already rewriting the stack trace in that scenario, putting <anonymous> in place of the filename.

That covers exceptions, but not the Babel compiler itself throwing an error:

./bin/coffee --transpile --eval '`import path`' SyntaxError: ~/Sites/coffeescript/.: 'import' and 'export' may only appear at the top level (2:2) 1 | (function() { > 2 | import path; | ^ 3 | 4 | 5 | }).call(this); at Parser.pp$5.raise (~/Sites/coffeescript/node_modules/babel-core/node_modules/babylon/lib/index.js:4454:13) at Parser.pp$1.parseStatement (~/Sites/coffeescript/node_modules/babel-core/node_modules/babylon/lib/index.js:1877:16) at Parser.pp$1.parseBlockBody (~/Sites/coffeescript/node_modules/babel-core/node_modules/babylon/lib/index.js:2268:21)

So the filename appears next to the SyntaxError here, but I think /. here is fine. I think it might make a bit more sense than ~/Sites/coffeescript/./<anonymous>, which would cause some head scratching since that path wouldn’t resolve. Also I’m still nervous about so much reliance on filename, which is essentially undocumented; what if a future change to filename means that it needs to be a resolvable path? Then suddenly passing it ~/Sites/coffeescript/./<anonymous> breaks --transpile.

should Babel use the .babelrc if it doesn’t know a filename?

Yes, to handle the --eval or stdin cases. Is there a reason we shouldn’t allow --transpile to work in those scenarios? I’d rather Babel find its options the usual way based on the current folder for these cases, than have someone pass in JSON as a command-line argument.

So the filename appears next to the SyntaxError here, but I think /. here is fine.

Sounds good, just figured I'd mention it.

Also I’m still nervous about so much reliance on filename, which is essentially undocumented

Babel's documentation just isn't great but that doesn't mean we're anxious to break our users. We do our absolute best to only introduce breaking changes on major versions. There's zero chance we'd go from treating filename as a maybe-existing file to a guaranteed-existing file without a major version bump.

I’d rather Babel find its options the usual way based on the current folder for these cases

I think it's your call. I don't feel strongly. I'll say, the reason in my mind that having the filename is important for config resolution is for cases like this:

project/ src/ index.js .babelrc

given

babel src/index.js // vs babel < src/index.js

because the first will find the .babelrc and the second one won't.

For Babel, we just skip config resolution entirely unless we know the actual location to search from.

@loganfsmyth Thanks for all these notes. I think the --eval and stdin use cases are edge enough that I’d rather pass the current folder as the filename so that transpilation works for these options, rather than force someone to save a test.coffee file or similar. Most likely someone compiling code inline via the CLI is just testing things, and this saves them some hassle. We should know (and pass along) the filename for all common use cases, as far as I’m aware, whether they’re compiling a single file or a folder tree.

Do you mind adding a comment in the Babel source code near where filename is parsed that upstream tools are expecting this functionality out of it? Perhaps with a link to this PR or to #4713 😄 Any little bit helps.

I don't think adding a comment isn't gonna make it any clearer. It's a core piece of the way Babel processes configs. The fact that it exists in the first place by definition means that upstream things rely on it heavily and changing it would break them.

jashkenas · 2017-09-25T18:41:40Z

Looks pretty great! Thanks, @GeoffreyBooth.

My only minor quibble is that I'd probably try to trim down some of the language and redundancies around the transpilation instructions. Since (either now, or very shortly) you won't need Babel in order to use compiled CS2 for everything with the exception of JSX — it seems like we lean on folks a little too hard with instructions for installing it.

GeoffreyBooth · 2017-09-25T19:08:07Z

@jashkenas will do, though it doesn’t look like Node is supporting import or export anytime soon: https://medium.com/the-node-js-collection/an-update-on-es6-modules-in-node-js-42c958b890c (from February 2017: “We’re currently looking at around a year at least.”).

@jashkenas and @lydell how do you feel about the use case of --transpile where we don’t know the filename?

Are there scenarios besides --eval or stdin that I’m not thinking of, where the compiler doesn’t know the filename?
Should we perhaps disallow --transpile in such scenarios (see @loganfsmyth’s comments above) or allow it while passing Babel the current folder to use to search for its options?

This is irrelevant for the Node API version of transpile, which always requires the options passed in directly as an object.

lydell · 2017-09-25T19:14:05Z

I vote for using process.cwd for --eval and stdin.

jashkenas · 2017-09-25T19:17:23Z

@jashkenas will do, though it doesn’t look like Node is supporting import or export anytime soon: https://medium.com/the-node-js-collection/an-update-on-es6-modules-in-node-js-42c958b890c (from February 2017: “We’re currently looking at around a year at least.”).

Very true, but if you're writing for Node with CS2 today, you can just use require. Much better than than transpiling w/Babel for Node.

process.cwd for --transpile with no file sounds fine.

Idle question, but does the coffee REPL work with --transpile? Should it?

GeoffreyBooth · 2017-09-26T01:23:16Z

does the coffee REPL work with --transpile? Should it?

No, and I think not at the moment. I don’t see any great need for REPL users to be able to use import statements or JSX. It might be nice to throw an error when someone runs coffee --transpile --interactive or coffee --transpile, but I don’t think that needs to be part of this PR.

…pile-fixes

GeoffreyBooth · 2017-09-26T01:57:50Z

@jashkenas I’ve rewritten the transpilation section to make it slightly shorter and hopefully clearer:
http://rawgit.com/GeoffreyBooth/coffeescript/transpile-fixes/docs/v2/index.html#transpilation

I think the section should be somewhat hand-holdy, as there are a significant number of people out there who use the coffee command as the entirety of their build chain specifically because they want to avoid complexity, and I don’t want to scare them off from the complexity that --transpile adds to their process.

@lydell and @jashkenas since you are good with the process.cwd approach, aside from the docs are there any other notes for this PR?

loganfsmyth · 2017-09-26T05:15:58Z

src/command.coffee

+      if filename
+        opts.transpile.filename = filename
+      else
+        opts.transpile.filename = base or process.cwd()


General feedback, this can all be

unless opts.transpile.filename opts.transpile.filename = filename or path.resolve(base or process.cwd(), "(anonymous)")

if you wanted to avoid manually fiddling with the path separators and wanted to not use .

Okay, I can get behind this, though it should be <anonymous> (brackets, not parentheses) to match the stack traces for compiled strings.

./bin/coffee --transpile --eval '<div/>' SyntaxError: ~/coffeescript/<anonymous>: Unexpected token (2:2) 1 | (function() { > 2 | <div />; | ^

…etter resolving of paths

GeoffreyBooth requested review from jashkenas and lydell September 23, 2017 20:21

GeoffreyBooth added 6 commits September 24, 2017 11:13

Show an appropriate error if the user tries to use --transpile via th…

3378f8c

…e CLI and babel-core isn’t installed

Update documentation around global/local

b6b536e

Fix jashkenas#4713: Use Babel’s built-in filename option to let Bab…

b56eb8d

…el search for its options, rather than us doing so

Improve transpilation docs

e949062

Colons are good

bb5ccc1

Docs cleanup

7259265

GeoffreyBooth force-pushed the transpile-fixes branch from 3528d8f to 7259265 Compare September 24, 2017 18:15

Repository owner deleted a comment from lydell Sep 24, 2017

loganfsmyth reviewed Sep 25, 2017

View reviewed changes

GeoffreyBooth added 2 commits September 25, 2017 18:30

Merge branch 'master' of github.com:jashkenas/coffeescript into trans…

ae3f379

…pile-fixes

Rewrite transpilation docs

071b4e3

lydell approved these changes Sep 26, 2017

View reviewed changes

loganfsmyth reviewed Sep 26, 2017

View reviewed changes

Better identifier for compiled scripts that didn’t come from files; b…

a5cdb30

…etter resolving of paths

GeoffreyBooth merged commit 9df1457 into jashkenas:master Sep 26, 2017

GeoffreyBooth deleted the transpile-fixes branch September 26, 2017 16:20

GeoffreyBooth mentioned this pull request Sep 26, 2017

2.0.1 #4719

Merged

Fix #4703, 4713: Transpile fixes #4717

Fix #4703, 4713: Transpile fixes #4717

Uh oh!

Conversation

GeoffreyBooth commented Sep 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lydell commented Sep 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GeoffreyBooth commented Sep 24, 2017

Uh oh!

GeoffreyBooth commented Sep 24, 2017

Uh oh!

lydell commented Sep 24, 2017

Uh oh!

GeoffreyBooth commented Sep 24, 2017

Uh oh!

loganfsmyth Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GeoffreyBooth Sep 25, 2017

Choose a reason for hiding this comment

Uh oh!

loganfsmyth Sep 25, 2017

Choose a reason for hiding this comment

Uh oh!

GeoffreyBooth Sep 26, 2017

Choose a reason for hiding this comment

Uh oh!

loganfsmyth Sep 26, 2017

Choose a reason for hiding this comment

Uh oh!

jashkenas commented Sep 25, 2017

Uh oh!

GeoffreyBooth commented Sep 25, 2017

Uh oh!

lydell commented Sep 25, 2017

Uh oh!

jashkenas commented Sep 25, 2017

Uh oh!

GeoffreyBooth commented Sep 26, 2017

Uh oh!

GeoffreyBooth commented Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

loganfsmyth Sep 26, 2017

Choose a reason for hiding this comment

Uh oh!

GeoffreyBooth Sep 26, 2017

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

GeoffreyBooth commented Sep 23, 2017 •

edited

Loading

lydell commented Sep 24, 2017 •

edited

Loading

loganfsmyth Sep 25, 2017 •

edited

Loading

GeoffreyBooth commented Sep 26, 2017 •

edited

Loading