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

:arrow_up: coffee-script #13731

Merged
merged 1 commit into from Oct 26, 2017

Conversation

Projects
None yet
5 participants
@Ingramz
Contributor

Ingramz commented Feb 2, 2017

Just a friendly reminder to occasionally update coffee-script.

Allows new syntax to be used and also provides better source maps.

After merging, make sure to clear compile-cache for updated source maps to take effect.

Changelogs can be found at http://coffeescript.org/#1.12.0 and then by scrolling up.

@50Wliu 50Wliu added the needs-testing label Feb 2, 2017

@ericcornelissen

This comment has been minimized.

Show comment
Hide comment
@ericcornelissen

ericcornelissen Feb 17, 2017

Works fine on Windows 10 (x64)

  • Run tests successfully
  • Build
  • Use Atom after building

ericcornelissen commented Feb 17, 2017

Works fine on Windows 10 (x64)

  • Run tests successfully
  • Build
  • Use Atom after building
@Ingramz

This comment has been minimized.

Show comment
Hide comment
@Ingramz

Ingramz Apr 17, 2017

Contributor

I don't know what's wrong with CI but I've had the build running for a while now and diff against the parent build only shows differences in sourcemaps, so I would say that it is safe to merge this.

Contributor

Ingramz commented Apr 17, 2017

I don't know what's wrong with CI but I've had the build running for a while now and diff against the parent build only shows differences in sourcemaps, so I would say that it is safe to merge this.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Oct 26, 2017

Contributor

Hey @Ingramz. Thanks for this.

Unfortunately, after watching the ecosystem evolve we have realized that never should have baked transpilation into Atom. If we upgrade the bundled CoffeeScript transpiler, there is a high probability of breaking a bunch of existing code.

Long term, the solution is for us to fix our package publishing system to allow transpiled assets to be published along with the release in addition to source files that are checked into Git. We haven't gotten there yet 😞.

In the short term, to use a newer CoffeeScript or any other language you want, you can bundle a custom transpiler with your package that will cache the transpiled sources the first time the package is started. This is great for development but it's unfortunate that it forces you to bundle the transpiler as a dependency of the package which increases its storage footprint and install time. If you can accept that trade-off, then the github package is a good example of how a custom transpiler can be used. (/cc @BinaryMuse)

My apologies for this situation. Baking in transpilation was a short-sighted design decision I made early on in this project and we really need to do the work to allow artifacts to be published with packages, but haven't.

Contributor

nathansobo commented Oct 26, 2017

Hey @Ingramz. Thanks for this.

Unfortunately, after watching the ecosystem evolve we have realized that never should have baked transpilation into Atom. If we upgrade the bundled CoffeeScript transpiler, there is a high probability of breaking a bunch of existing code.

Long term, the solution is for us to fix our package publishing system to allow transpiled assets to be published along with the release in addition to source files that are checked into Git. We haven't gotten there yet 😞.

In the short term, to use a newer CoffeeScript or any other language you want, you can bundle a custom transpiler with your package that will cache the transpiled sources the first time the package is started. This is great for development but it's unfortunate that it forces you to bundle the transpiler as a dependency of the package which increases its storage footprint and install time. If you can accept that trade-off, then the github package is a good example of how a custom transpiler can be used. (/cc @BinaryMuse)

My apologies for this situation. Baking in transpilation was a short-sighted design decision I made early on in this project and we really need to do the work to allow artifacts to be published with packages, but haven't.

@nathansobo nathansobo closed this Oct 26, 2017

@nathansobo nathansobo reopened this Oct 26, 2017

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Oct 26, 2017

Contributor

Okay, after further discussion with @Ingramz and other maintainers, I've changed my mind on this.

Bundled transpilation is not something we plan to stick with over the long term, but this minor upgrade seems to be low risk. If it breaks packages, which seems unlikely, we can lint for the offending construct across the ecosystem.

I just want to say right now that merging this upgrade is not a promise to merge future upgrades. We'll have to take it on a case-by-case basis and judge the risk in each scenario. Specifically, it's extremely unlikely that we'll be able to upgrade bundled transpilation to CoffeeScript 2.0.

Contributor

nathansobo commented Oct 26, 2017

Okay, after further discussion with @Ingramz and other maintainers, I've changed my mind on this.

Bundled transpilation is not something we plan to stick with over the long term, but this minor upgrade seems to be low risk. If it breaks packages, which seems unlikely, we can lint for the offending construct across the ecosystem.

I just want to say right now that merging this upgrade is not a promise to merge future upgrades. We'll have to take it on a case-by-case basis and judge the risk in each scenario. Specifically, it's extremely unlikely that we'll be able to upgrade bundled transpilation to CoffeeScript 2.0.

@nathansobo nathansobo merged commit e34d167 into atom:master Oct 26, 2017

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
ci/circleci Your tests passed on CircleCI!
Details

@nathansobo nathansobo self-assigned this Oct 26, 2017

@Ingramz

This comment has been minimized.

Show comment
Hide comment
@Ingramz

Ingramz Oct 26, 2017

Contributor

Part of my promise to @nathansobo during that discussion was to check for compilation output differences between the old and the new version across all the packages.

Compilation differences between 1.11.1 and 1.12.7 were found in following .coffee files using atom-package-downloader and find . -name "*.coffee" -type f. This resulted in 13242 files that needed to be checked. I crafted a script that ran coffeescript.compile(contents) for both versions and compared their outputs whether they are equal or not. Files that failed to compile with 1.11.1 were excluded.

Differences were found in following files:

atom-beautify/src/beautifiers/index.coffee
atom-beautify/src/beautifiers/sqlformat.coffee
atom-gibo/lib/atom-gibo.coffee
atom-latex/spec/main-spec.coffee
cubic-bezier/lib/cubic-bezier-curve.coffee
ever-notedown/lib/markdown-helper.coffee
file-header/lib/file-header.coffee
find-and-replace-fix/lib/project/results-view.coffee
forcedotcom-builder/lib/build.coffee
git-control/lib/dialogs/flow-dialog.coffee
git-control/lib/views/file-view.coffee
git-controlhub/lib/dialogs/flow-dialog.coffee
greppen-grok/lib/greppen-grok-model.coffee
imdone-atom-github/lib/plugin.coffee
quick-editor/lib/quick-editor.coffee
rot13/lib/rot13.coffee
sf-tools/lib/build.coffee
simple-git/lib/editor.coffee
super-duplicate/lib/super-duplicate.coffee
tnote/lib/tnote.coffee
ult-terminal/lib/term-view.coffee
you-complete-me/lib/get-issues.coffee

List is small enough that I can go through those by hand.

Contributor

Ingramz commented Oct 26, 2017

Part of my promise to @nathansobo during that discussion was to check for compilation output differences between the old and the new version across all the packages.

Compilation differences between 1.11.1 and 1.12.7 were found in following .coffee files using atom-package-downloader and find . -name "*.coffee" -type f. This resulted in 13242 files that needed to be checked. I crafted a script that ran coffeescript.compile(contents) for both versions and compared their outputs whether they are equal or not. Files that failed to compile with 1.11.1 were excluded.

Differences were found in following files:

atom-beautify/src/beautifiers/index.coffee
atom-beautify/src/beautifiers/sqlformat.coffee
atom-gibo/lib/atom-gibo.coffee
atom-latex/spec/main-spec.coffee
cubic-bezier/lib/cubic-bezier-curve.coffee
ever-notedown/lib/markdown-helper.coffee
file-header/lib/file-header.coffee
find-and-replace-fix/lib/project/results-view.coffee
forcedotcom-builder/lib/build.coffee
git-control/lib/dialogs/flow-dialog.coffee
git-control/lib/views/file-view.coffee
git-controlhub/lib/dialogs/flow-dialog.coffee
greppen-grok/lib/greppen-grok-model.coffee
imdone-atom-github/lib/plugin.coffee
quick-editor/lib/quick-editor.coffee
rot13/lib/rot13.coffee
sf-tools/lib/build.coffee
simple-git/lib/editor.coffee
super-duplicate/lib/super-duplicate.coffee
tnote/lib/tnote.coffee
ult-terminal/lib/term-view.coffee
you-complete-me/lib/get-issues.coffee

List is small enough that I can go through those by hand.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Oct 26, 2017

Contributor

Fabulous. Thanks for the quick follow-up.

Contributor

nathansobo commented Oct 26, 2017

Fabulous. Thanks for the quick follow-up.

@Ingramz

This comment has been minimized.

Show comment
Hide comment
@Ingramz

Ingramz Oct 26, 2017

Contributor

Differences are mostly that code which compiled to ternary expressions, the conditional is now surrounded by parentheses, which is harmless.

There are three files that have a different change, which need to be looked at case by case:
atom-latex/spec/main-spec.coffee (original)
quick-editor/lib/quick-editor.coffee (original)
you-complete-me/lib/get-issues.coffee (original)

Contributor

Ingramz commented Oct 26, 2017

Differences are mostly that code which compiled to ternary expressions, the conditional is now surrounded by parentheses, which is harmless.

There are three files that have a different change, which need to be looked at case by case:
atom-latex/spec/main-spec.coffee (original)
quick-editor/lib/quick-editor.coffee (original)
you-complete-me/lib/get-issues.coffee (original)

@GeoffreyBooth

This comment has been minimized.

Show comment
Hide comment
@GeoffreyBooth

GeoffreyBooth Oct 27, 2017

Please change your dependency to coffeescript rather than coffee-script. coffeescript is the primary module now for CoffeeScript.

GeoffreyBooth commented Oct 27, 2017

Please change your dependency to coffeescript rather than coffee-script. coffeescript is the primary module now for CoffeeScript.

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