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

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

Closed
SnarkBoojum opened this issue Mar 16, 2020 · 11 comments
Closed

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

SnarkBoojum opened this issue Mar 16, 2020 · 11 comments

Comments

@SnarkBoojum
Copy link

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
Copy link
Contributor

kzc commented Mar 24, 2020

A number of bug fixes and memory efficiency improvements were made in magic-string. The new buble test results with [email protected] 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
Copy link
Collaborator

mourner commented Mar 26, 2020

@kzc see #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
Copy link
Contributor

kzc commented Mar 26, 2020

@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
Copy link
Collaborator

mourner commented Mar 26, 2020

@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
Copy link
Contributor

kzc commented Mar 26, 2020

@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
Copy link
Collaborator

mourner commented Mar 26, 2020

@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
Copy link
Collaborator

mourner commented Mar 26, 2020

Landed in master

@mourner mourner closed this as completed Mar 26, 2020
@kzc
Copy link
Contributor

kzc commented Mar 26, 2020

@mourner Do you plan to make a new release?

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

@mourner
Copy link
Collaborator

mourner commented Mar 26, 2020

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

@mourner
Copy link
Collaborator

mourner commented Mar 26, 2020

@kzc
Copy link
Contributor

kzc commented Mar 26, 2020

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.

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

No branches or pull requests

3 participants