bublejs / buble

https://buble.surge.sh
MIT License
871 stars 67 forks source link

New magic-string 0.25.7 makes two tests fail in buble #248

Closed SnarkBoojum closed 4 years ago

SnarkBoojum commented 4 years ago

Hi,

I wanted to update Debian's package for magic string to version 0.25.7, but didn't upload the result yet because it turns out it breaks two tests in buble.

I'm attaching the relevant problems. buble_with_magic_string_0.25.7.txt

kzc commented 4 years ago

A number of bug fixes and memory efficiency improvements were made in magic-string. The new buble test results with magic-string@0.25.7 provide more accurate higher precision source maps.

Both the failing tests you listed use the same input:

const answer = () => 42;

and produce the same output:

var answer = function () { return 42; };

and same final source map. The only difference is that one test produces an inline source map, and the other an external source map file.

You can see the source map improvement by comparing the original test result for /test/cli/creates-inline-sourcemap versus the new test result. You will notice that the new sourcemap has an extra mapping 25->1:18 from => to { return. Hover your mouse over each token in the source map visualizer to study it.

Just create a new PR to update the expected buble test results with the actuals. It would be nice if a new Buble release could be made since there appears to be other unpublished committed changes since the last release.

mourner commented 4 years ago

@kzc see https://github.com/bublejs/buble/issues/241 — I would like to release a new version but make it v1.0, dropping old Node and upgrading all dependencies — does this sound good? We could do a bump release but there's no way to verify if it works on old Node versions/deps because master builds already fail on them for whatever reason.

kzc commented 4 years ago

@mourner I'm not sure deprecating old Node versions is a good idea. It defeats the point of the project, doesn't it? I just did a clean git clone of buble from latest master, ran yarn, updated the expecteds with the actuals for the 2 failing sourcemap tests and it ran all the mocha tests successfully on Node 4 and 6. Yes, running yarn (or npm install) generates a lot of deprecation warnings, but it still works. I think doing a bump release is the way to go.

mourner commented 4 years ago

@kzc the test262 tests are failing on CI though. Deprecating old Node is a good idea because the point of the project is to support legacy browsers like IE11 first and foremost; old Node versions shouldn't be run anywhere at all since they are not maintained even for critical security issues, and supporting them puts a huge burden on maintenance of this project, as discussed in the linked tickets.

kzc commented 4 years ago

@mourner It's your call. I just assumed that the commits that occurred after the last release were vetted in some way - at the very least included new passing tests. It would be a shame not to release those fixes.

npm test runs fine. I didn't check the test262 results because I assumed that Buble is far from compliant.

Regarding maintenance burden, I thought this was a defunct project - no new features. Updating the test results to for the latest version of magic-string is just a minor thing.

mourner commented 4 years ago

@kzc perhaps you're right — I just went through deps in #250 and it seems like at least direct dependencies weren't updated to versions incompatible with old Node, so we can do a minor bump at least (v0.20.0).

mourner commented 4 years ago

Landed in master

kzc commented 4 years ago

@mourner Do you plan to make a new release?

The last one was '0.19.8': '2019-07-03T08:59:31.653Z'.

mourner commented 4 years ago

@kzc yes, after merging a few PRs that were stuck unmerged because of the failing CI on older Node versions.

mourner commented 4 years ago

Released v0.20.0. https://github.com/bublejs/buble/blob/master/CHANGELOG.md#0200-2020-03-26

kzc commented 4 years ago

Thanks @mourner.

fwiw, I can confirm once buble 0.20.0 is built with Node v12 from sources that the mocha tests run successfully with Node v4 and v6 if the older versions of mocha and rimraf are used:

    "mocha": "^5.2.0",
    "rimraf": "^2.6.3",
$ bin/buble -v
Bublé version 0.20.0
$ node-v4.2.1 node_modules/.bin/mocha test/test.js && echo success
...
  539 passing (2s)
  2 pending

success
$ node-v6.9.0 node_modules/.bin/mocha test/test.js && echo success
...
  539 passing (2s)
  2 pending

success

Of course going forward that may not be the case.