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

Fix CoffeeScript source map composition #8298

Merged
merged 1 commit into from Feb 10, 2017

Conversation

Projects
None yet
4 participants
@klaussner
Collaborator

klaussner commented Jan 29, 2017

Fixes missing CoffeeScript source maps (#8281/#8285).

  • CoffeeScript has an inlineMap option that sets the sourcesContent property. However, it also includes the source map at the end of the compiled code. Since we have to process the compiled code afterwards, we can't use this option and have to set the sourcesContent manually. There is an issue in the CoffeeScript repo for more flexible source map options: jashkenas/coffeescript#4342.
  • The SourceMapGenerator was created incorrectly (its constructor doesn't accept a SourceMapConsumer), causing the generated source map to be empty.

A note on source map size: the produced source maps are correct but larger than necessary because of the way applySourceMap works. If there is no mapping from compiled CoffeeScript code to its source (return statements aren't mapped, for example), applySourceMap keeps the mappings from app.js to the compiled code in the source map. Therefore, the original CoffeeScript code as well as its compiled version become part of the sourcesContent property. The source map size could be reduced by ignoring these mappings but I think it's not worth writing a custom version of applySourceMap just for that.

@abernix @mjmasn @GeoffreyBooth

@abernix

This comment has been minimized.

Member

abernix commented Jan 31, 2017

Awesome! Your logic and comments make sense to me. Probably worth linking specifically to your comment on the other issue too for others to review.

Any chance of getting some tests included on this? I feel like we've regressed on this CoffeeScript source maps multiple times now and would like to try to harden that up.

@mjmasn, @GeoffreyBooth: Confirmation on whether this further solves the problem?

// Reference the compiled CoffeeScript file so that `applySourceMap`
// below can match it with the source map produced by the CoffeeScript
// compiler.
doubleRoastedCoffee.sourceMap.sources[0] += '.js';

This comment has been minimized.

@benjamn

benjamn Feb 1, 2017

Member

Maybe set

doubleRoastedCoffee.sourceMap.sources[0] = this._outputFilePath(inputFile);

if it's the same path? I'm pretty sure it is, but haven't checked.

This comment has been minimized.

@klaussner

klaussner Feb 3, 2017

Collaborator

_outputFilePath returns the right path but without leading /.

@klaussner klaussner force-pushed the klaussner:issue-8281 branch from c5d0647 to 6ccaee2 Feb 3, 2017

@klaussner

This comment has been minimized.

Collaborator

klaussner commented Feb 3, 2017

I think I need some help with the tests. 😅 If I understand correctly, meteor test-packages runs the tests in an application environment, so they won't have access to the build plugin code and the source map from compileOneFile.

@benjamn

benjamn approved these changes Feb 8, 2017

My previous concerns have been addressed! Would still be good to figure out how to test this, per #8298 (comment).

@GeoffreyBooth

This comment has been minimized.

Contributor

GeoffreyBooth commented Feb 9, 2017

@benjamn I’ve been meaning to find time to look into this, but I haven’t yet. The only idea I had regarding testing was to do a $.get or HTTP.get call for the generated file (i.e. the .js file output by the coffeescript package) and inspecting that file for the source map. Is there some way to know what the URL should be for the .js file generated as part of the tests? Or file path, if this is easier to do server-side? (Presumably we don’t need to test for source maps in both places, either it’s in one or it’s in neither.)

@klaussner

This comment has been minimized.

Collaborator

klaussner commented Feb 10, 2017

What do you think about creating another package, coffeescript-compiler, that exports only the CoffeeScript compiler and using that in the coffeescript package, similar to babel-compiler and ecmascript? Then we would have access to code and source maps without running the build plugin.

@benjamn

This comment has been minimized.

Member

benjamn commented Feb 10, 2017

I would support that split, though it doesn't necessarily need to happen before Meteor 1.4.3, as long the new version of coffeescript (depending on coffeescript-compiler) is patch-compatible with the current version.

Questions of versioning are about to become somewhat more important, since 1.4.3 will be going back to constraining core packages to be patch-compatible with the version that was current when the Meteor release was published:

Given this change, it might make sense to move packages/coffeescript to packages/non-core/coffeescript, so that it won't be constrained by the release (similar to how it works now, post-1.4 version unpinning).

@benjamn

This comment has been minimized.

Member

benjamn commented Feb 10, 2017

Another way of asking the same question: would coffeescript benefit in any way from being constrained by the Meteor release? Or would you rather be able to bump the version whenever and however you like? If the second option is more appealing, then we should probably do the move to packages/non-core/coffescript prior to 1.4.3.

One way coffeescript might benefit: if coffeescript remains a core package, then, when developers update Meteor, the coffeescript package will be forced to upgrade. Currently there is no pressure for developers to upgrade.

Perhaps this discussion should happen on a different thread…

@GeoffreyBooth

This comment has been minimized.

Contributor

GeoffreyBooth commented Feb 10, 2017

I guess the question is, do any Meteor core packages depend on coffeescript? I assume not?

Something else to keep in mind is that CoffeeScript 2.0.0-alpha1 is imminent, and will be a breaking-change release of CoffeeScript. The breaking changes are minimal, but could affect people, especially people that use a lot of the advanced features of CoffeeScript classes. This might be a reason to let people control what version of CoffeeScript to install into a project.

@benjamn benjamn changed the base branch from devel to release-1.4.3 Feb 10, 2017

@benjamn benjamn added this to the Release 1.4.3 milestone Feb 10, 2017

@benjamn

This comment has been minimized.

Member

benjamn commented Feb 10, 2017

It sounds like the balance of arguments favors keeping coffeescript independent from the release (i.e., moving it into packages/non-core). Given that, I'm going to merge this PR into the release-1.4.3 branch, then git mv coffeescript non-core/ so that Meteor 1.4.3 won't constrain the coffeescript version (and thus won't prevent upgrading to 2.0.0).

@benjamn benjamn merged commit 63e9160 into meteor:release-1.4.3 Feb 10, 2017

2 checks passed

CLA Author has signed the Meteor CLA.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

benjamn added a commit that referenced this pull request Feb 10, 2017

Move coffeescript into packages/non-core.
See discussion on #8298, starting with this comment:
#8298 (comment)

@benjamn benjamn referenced this pull request Feb 10, 2017

Merged

Release 1.4.3 #8123

@GeoffreyBooth

This comment has been minimized.

Contributor

GeoffreyBooth commented Feb 10, 2017

Thanks @benjamn

@benjamn

This comment has been minimized.

Member

benjamn commented Feb 14, 2017

Just published coffeescript@1.12.3_1 using the latest recommended Meteor version, 1.4.2.7.

I tested source maps manually and they seemed to work!

abernix added a commit that referenced this pull request Feb 23, 2017

abernix added a commit that referenced this pull request Mar 1, 2017

@GeoffreyBooth

This comment has been minimized.

Contributor

GeoffreyBooth commented Jul 27, 2017

@benjamn I noticed that since moving coffeescript to non-core, it’s not included in the tests that run via ./meteor test-packages. I eventually figured out that it can still be tested via ./meteor test-packages packages/non-core/coffeescript, but you might want to update the regular test-packages command to also test the non-core packages. They may not be restricted to a particular Meteor release version, but they’re “core enough” to be part of the Meteor repo and presumably used by a great many Meteor apps. I would hope your CI server runs these tests as part of its builds.

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