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

CoffeeScript compiler should pass code within backticks to ecmascript package #6691

Conversation

GeoffreyBooth
Copy link
Contributor

This pull request implements the following for the core coffeescript package:

  • If CoffeeScript source for a parsed .coffee file contains backticks, the CoffeeScript output is passed through ecmascript. This enables ES2015 written within backticks to work, while avoiding the performance hit of sending all CoffeeScript code through ecmascript. Closes [1.3-modules-beta.4] modules not working with coffeescript #6000.
  • Two new tests that verify that import statements wrapped in backticks work, whether importing from an external package or from a local .js file that has an export statement. All the old package tests still pass.
  • Bumped the imported NPM coffee-script module version from 1.9.2 to 1.10.0 (released 2015-09-03 and considered stable). Closes Bump coffeescript to 1.9.3 #4807.
  • Bumped the core coffeescript package version from 1.0.16 to 1.1.0.

Besides the included tests, I’ve created a demo app that uses this new package.

@laosb
Copy link
Contributor

laosb commented Apr 4, 2016

Neat!

GeoffreyBooth and others added 20 commits May 8, 2016 13:30
# Conflicts:
#	packages/coffeescript/package.js
Together with c18c1f5, this commit is a
big step towards liberating Meteor from Node 0.10.

Part of meteor#5124.
This is an adaptation of @tmeasday's 74230be test that he created for meteor#5680.  Due to occasional failures, it now uses sinon's `lolex` npm library to allow us to control the setTimeout/setInterval within the test itself, providing for tick-accurate testing. Also nifty because it allows the test to finish in less time than it actually takes.
Note that this may be a breaking change for server-side code that uses
Promise.denodeify, Promise.nodeify, Function.prototype.async, or
Function.prototype.asyncApply, since those APIs have been removed for the
sake of simplicity.
TODO Remember to bump $BUNDLE_VERSION before the next release.
@tmeasday
Copy link
Contributor

@GeoffreyBooth also note that the selftest.js:646 failure is due to a timeout which we've bumped recently so I think you can ignore (or do the same to work around)

@GeoffreyBooth
Copy link
Contributor Author

@benjamn I’m happy to work on this, should I just merge release-1.3.3 into my branch?

Ben Newman and others added 8 commits May 25, 2016 00:01
The most substantive change here is the additional call to
colonConverter.convert, which fixes meteor#7096.

In the future, I would like to unify this implementation with similar
logic in tools/static-assets/server/npm-require.js.
@GeoffreyBooth
Copy link
Contributor Author

Okay, I’ve revised per @benjamn:

  • ecmascript has been replaced with babel-compiler for compilation of code within backticks. (ecmascript still floats around for compiling the coffeescript package’s own code, and for compiling packages/coffeescript/tests/es2015_module.js during Package.onTest.)
  • One instance of BabelCompiler is created for each CoffeeCompiler instance.
  • babelCompiler.processOneFileForTarget is used for backtick processing.
  • If BabelCompiler returns an object with a .data property, this new code replaces output.js. I ignore any .sourceMap property, because the original CoffeeScript sourcemap contains the ES2015 code, so there’s no need to do anything with babel-compiler’s sourcemap—it should be identical to the parts of the CoffeeScript sourcemap within backticks.
  • Not requested, but I bumped the version of source-map that coffeescript depends on.

I also took out the try/catch block, as it appears that babel-compiler is already catching whatever exceptions we want caught.

Since I merged the release-1.3.3 branch into my coffeescript-backticks-ecmascript branch to get @benjamn’s latest code, now of course this commit shows hundreds of changes as its target is still devel. Can you process this, or should I open a new PR against release-1.3.3? cc @tmeasday

@GeoffreyBooth
Copy link
Contributor Author

Now that Meteor 1.3.3-beta.2 is out, and includes the release-1.3.3 commit @benjamn mentioned above, I updated my demo app that uses this updated coffeescript package. With 1.3.3-beta.2 and this version of coffeescript, the ES2015 imports work within backticks.

@benjamn
Copy link
Contributor

benjamn commented Jun 2, 2016

Great, thanks for taking the time to make these updates. I'll take a closer look at this tomorrow.

@benjamn
Copy link
Contributor

benjamn commented Jun 2, 2016

Ok, I've rebased these changes with a few cleanups (leaving your authorship intact), and pushed them to release-1.3.3, as you can see here. Thanks so much for working on this!

@benjamn benjamn closed this Jun 2, 2016
@tmeasday
Copy link
Contributor

tmeasday commented Jun 2, 2016

Thanks for bearing with us on this one @GeoffreyBooth !

@GeoffreyBooth
Copy link
Contributor Author

Thanks!

@GeoffreyBooth
Copy link
Contributor Author

@benjamn Do you mind explaining 73ed3ff, where you added code to merge the CoffeeScript compiler’s source map with the Babel compiler’s source map? Why was that necessary?

I’m considering adding Babel transpilation as an option to the CoffeeScript compiler itself; see jashkenas/coffeescript#4696. The idea is that it’s not the easiest challenge for people to figure out how to set up a build chain where CoffeeScript’s output gets piped through Babel, so if we can simplify that part, we might as well. And the CoffeeScript compiler should especially offer this if there’s special handling required for source maps to work properly, as I’m assuming there must be for you to have added that code. So . . . what problem does merging the source maps solve? I want to be able to write a test that fails if the source maps are unmerged, and passes if they are.

@GeoffreyBooth
Copy link
Contributor Author

GeoffreyBooth commented Sep 11, 2017

P.S. @benjamn did you see this? babel/babel#827 (comment)

@klaussner
Copy link
Contributor

Anwering #9090 (comment):

The inputSourceMap option was used in babel-compiler because JavaScript code is compiled twice (one pass for presets and one for plugins), similar to coffeescript-compiler. For some reason, this caused the problems reported in #8611 (broken source maps). The solution there was pretty simple because the compiler could just reuse the AST from the first Babel transformation step without creating an intermediate source map (meteor/babel#17).

However, I'm still not sure what exactly caused the source maps to be broken. The implementation of inputSourceMap is a little different from applySourceMap (used in coffeescript-compiler) but it should produce similar results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants