MattiasBuelens / web-streams-adapter

Adapters for converting between different implementations of WHATWG Streams
MIT License
32 stars 1 forks source link

CRLF line endings in built package on npm #12

Closed twiss closed 4 years ago

twiss commented 4 years ago

Hi @MattiasBuelens! Thanks for all your work on web-streams-*, as always :)

I saw https://github.com/MattiasBuelens/web-streams-polyfill/commit/60dd56c7b92ee2868ae2fb9914f75621a64d30f1; would it be possible for you to do something similar here and update the npm package to have LF line endings instead of CRLF?

For background, while switching from browserify to rollup, I ran into some "interesting"-to-debug errors whose ultimate cause was that my built package contained a mix of CRLF and LF, which made source-map-explorer assume it was CRLF-only, and split on lines by CRLF, and then complain that certain lines of code didn't contain code that they clearly did :)

MattiasBuelens commented 4 years ago

Issues with line endings are the worst... 😞

Anyway, I've just published v0.1.0-alpha.4 to npm, which should have LF endings everywhere. Could you give that a try? 🙂

twiss commented 4 years ago

Thanks! It seems like it still has a few left, from a util function that typescript is importing:

$ curl https://unpkg.com/@mattiasbuelens/web-streams-adapter@0.1.0-alpha.4/dist/web-streams-adapter.js | grep -n $'\r' | less
7:    /*! *****************************************************************************
8:    Copyright (c) Microsoft Corporation. All rights reserved.
9:    Licensed under the Apache License, Version 2.0 (the "License"); you may not use
10:    this file except in compliance with the License. You may obtain a copy of the
11:    License at http://www.apache.org/licenses/LICENSE-2.0
12:
13:    THIS CODE IS PROVIDED ON AN *AS IS* BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14:    KIND, EITHER EXPRESS OR IMPLIED, INCLUDING WITHOUT LIMITATION ANY IMPLIED
15:    WARRANTIES OR CONDITIONS OF TITLE, FITNESS FOR A PARTICULAR PURPOSE,
16:    MERCHANTABLITY OR NON-INFRINGEMENT.
17:
18:    See the Apache Version 2.0 License for specific language governing permissions
19:    and limitations under the License.
20:    ***************************************************************************** */
21:    /* global Reflect, Promise */
22:
23:    var extendStatics = function(d, b) {
24:        extendStatics = Object.setPrototypeOf ||
25:            ({ __proto__: [] } instanceof Array && function (d, b) { d.__proto__ = b; }) ||
26:            function (d, b) { for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p]; };
27:        return extendStatics(d, b);
28:    };
29:
30:    function __extends(d, b) {
31:        extendStatics(d, b);
32:        function __() { this.constructor = d; }
33:        d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());

I'm not really sure why this module has this function and web-streams-polyfill doesn't, though.. or why typescript is using CRLF for it. It seems like typescript is doing something simpler for extending classes in web-streams-polyfill, for some reason.

MattiasBuelens commented 4 years ago

Damnit! Looks like tslib in node_modules still has CRLF endings. I guess I should nuke my node_modules and do a clean npm install. 😛

I'll look into it later today.

I'm not really sure why this module has this function and web-streams-polyfill doesn't, though.. or why typescript is using CRLF for it. It seems like typescript is doing something simpler for extending classes in web-streams-polyfill, for some reason.

In web-streams-polyfill, there are no classes that extend from other classes, so TypeScript doesn't need to generate this helper. In this project, I do have such classes (here and here).

MattiasBuelens commented 4 years ago

Published as 0.1.0-alpha.5, should actually be fixed now. 😅

twiss commented 4 years ago

Yep, that fixed it! Thanks :pray: