Skip to content

Commit

Permalink
Make the SourceMapConsumer constructor return a promise
Browse files Browse the repository at this point in the history
  • Loading branch information
@fitzgen
fitzgen committed Dec 19, 2017
1 parent 264fcb4 commit f0e7420
Show file tree
Hide file tree
Showing 24 changed files with 312 additions and 25,597 deletions.
6 changes: 1 addition & 5 deletions .travis.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,7 @@ language: node_js
sudo: false

node_js:
- "0.10"
- "0.12"
- "4"
- "5"
- "6"
- "9"

cache:
directories:
Expand Down
Loading

5 comments on commit f0e7420

@stewx
Copy link

@stewx stewx commented on f0e7420 Feb 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this not require a major version number increase, given that it is a breaking change, no longer supporting versions of node lower than 8.0? This is causing issues for us now since we're on Node 6.10

@fitzgen
Copy link
Contributor Author

@fitzgen fitzgen commented on f0e7420 Feb 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It did. Below 1.0, a 0.X bump is considered a breaking change.

@yocontra
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is late to the game - but is there any documentation about why this change was made? Most of the tools that use source-map did not update to 0.7 because this was a major breaking API change that is difficult/impossible for people to pull in without having to do their own breaking API change (babel is still on 0.5 because they would need to make their entire API async to land this as one example).

Looking at the actual change I can't immediately see any benefits of making this async - I'm sure there were reasons for it but it would be good to have some documentation about it somewhere.

@loganfsmyth
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Best to continue this in #331 if you'd like, but the short answer is that this module now uses WASM, and loading WASM synchronously is mostly a no-go.

@yocontra
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@loganfsmyth Ah ok, makes complete sense - just looking at this commit in isolation made it seem unrelated, but if it's related to the WASM change that seems right. Thanks for the response.

Please sign in to comment.