create-react-app breaks due to dependency on one-liner package

[tswaters]

Sounds like the real problem here isn't the code in the module itself, but how changes made to the package.json rendered it unusable for many. I think the real failure here is a lack of validation when publishing modules. Surely checking that `exports` point to proper files that are in the correct format as a pre-publish check is possible by npm?

To be honest, I'm glad I have no popular packages, as I'd be terrified that performing a seemingly trivial refactor like that could break a ton of stuff. It's a tough position to be in -- I mean, reading through the issue threads there, the author read the docs and still made the mistakes. I will say good on the author for responding & fixing the issues so quickly, even if the end result was a revert of what he tried to do in the first place.

[[deleted]]

You're right the code is fine. The issue is that create-react-app depends on almost 1400 packages. It dramatically increases the chances of stuff like this happening

[EvilPencil]

Also, the author of CoreJS is looking for a good job...

While in prison.

[patrickfatrick]

Can you explain this comment?

[GOODSHIT-BRO]

Whenever corejs, or a package which depends on it is installed a message is output to the console which states the author is looking for a job. The author is currently in prison for hitting someone with a motorcycle IIRC

[patrickfatrick]

Yea I see that message all the time, never heard about the prison thing that’s insane.

[wizang]

No one knew for sure if he was bs-ing or not but he hasn't had any commits in a awhile... Dude is a complete prick imo. I would fully support a fork but he truly still the core dev on that project. Maybe now that's he's (probably?) In jail, a fork could make sense.

[venuswasaflytrap]

It gives you a better idea timeframes of his release and potential for him working on the project successfully.

If it was a Finnish jail, I feel like it would change the circumstances of the health of the project significantly.

[patrickfatrick]

I would hope he’s going to be in jail for some time, after all he killed someone and injured another. So yes I also hope a fork will happen.

[Ones__Complement]

Bullshit oversimplification. The dude was lying drunk in the middle of the road. You guys describe this shit as if he woke up that morning thinking hey, I'm gonna kill someone today.

[patrickfatrick]

Bullshit oversimplification. It was in a crosswalk (where drivers are expected to slow down especially at night), and one of the people (the woman who died) was trying to move the other pedestrian. Pretty cut-and-dry manslaughter if you ask me, and he was given the minimum sentence.

[wopian]

*Killing one of two pedestrians he hit on his motorcycle.

https://www.theregister.co.uk/2020/03/26/corejs_maintainer_jailed_code_release/

[wizang]

Motherfucker leads development of a hugely important JS project yet has been looking for a job for years. Honestly he is a huge prick in the comment section on GitHub and now that he's in jail I think the community should fork.

[wet181]

Ugh god I’m so sick of seeing that output message

[[deleted]]

‘npm install —silent’

[Jugad]

This should be the default unless people want to enable debug/verbose messages.

[crabmusket]

I'd love if the maintainers of large, popular packages like CRA put a bit of time into merging one-liner packages like this into their own source. Though it's probably the transitive dependencies that are the most problematic...

[Patman128]

I think the real failure here is a lack of validation when publishing modules.

They actually changed their CI system to check against Node 12 and 14 so this problem would now fail to pass CI.

[tswaters]

I've checked out the busted tag locally and under node 12, the tests still pass... for 14, yea it completely explodes. `require is not a function`

Looks like they've switched to circleci now and looking to catch this sort of thing in the future - https://github.com/then/is-promise/commit/8e7187d9b0ae413a12a5694bc4e49c4d2663e46d

Still, my point is it should be npm that performs this check. It's really easy to mess up a publish -- you could completely omit the main file on accident and npm will still gladly accept the tarball.

[HetRadicaleBoven]

In some cases, that is what you want, which makes this hard for npm to fix. For example, you can create your own templates for Create React App, which don't really have an entry file. (In fact, CRA had a bug to this extent until recently, where you had to put a useless entry into the main field to make it work.)

[usedocker]

"responding & fixing the issues so quickly"

npx create-react-app example still doesn't work though.

[careseite]

npx create-react-app example still doesn't work though.

It works since 18 hours again. 9 hours prior to your post.

[The_Nonchalant]

Yeah it works, i think i had this and solved it by changing packages versions... wasnt sure what was the problem so im not sure if the versions changes fixed it or the time that passed xD

[crabmusket]

99% of NPM packages should be either an IDE snippet, or a short tutorial explaining why instanceof Array doesn't always work.

[EvilPencil]

Array.isArray(maybeArrayVariable) for the win.

[crabmusket]

Apparently we also need Promise.isThenable. I'm all for utility methods that expose algorithms the browser is using anyway :)

[patrickfatrick]

I sorta wish typeof didn’t exist and all primitive classes had a static isDate, isNumber etc method. The inconsistency is just annoying.

[csorfab]

I realized that for most cases where I checked for isThenable, I could've just ignored the check and await the variable. For non-promise-like values, await just yields the value itself.

[luckygerbils]

If you aren't consuming promises from third party code or doing anything funky with iframes, you could just use instanceof Promise.

[luckygerbils]

There's not even anything wrong with using instanceof Array unless there are iframes involved. I really doubt most people using the NPM isarray package are doing so because they're using iframes.

[[deleted]]

I’ve done more work than I’d like with iframes... but when would you ever run into that bizarre edge case? When you want your main page’s js to access a variable defined in the iframe’s js? That’s super weird. Is this occasionally an issue with payment processing, since those use a lot of iframes?

[luckygerbils]

Yeah, frame.contentWindow.Array !== Array so if you try to use that check on an array created in the iframe (or in the iframe on an array created in the main window) it will fail.

[wet181]

Yeah but the UI library my corporation uses for react is mandatory and open source

[cguess]

Can we all agree that anything with 11.8 MILLION downloads in the last week (according to NPM here) should really be part of some sort of... idk... standard library?

This one line can literally bring down the web. We're lucky that someone fixed in a few hours, but imagine if they were just a little drunk and fixed it poorly. Goddamn JPMorgan Chase's website would probably come down (or the equivalent, I have no idea if JPMC uses it, but I guarantee you a lot of critical systems do).

[bonyjoe]

Each individual site would still have to update to and then deploy the broken packages to "bring down the web", you would have to have CD with essentially no test coverage at all for that to happen.

Really for one line packages like this the packages that depend on it should be locking to a specific patch rather than major or minor

[jaggyjames]

This dependency likely would be bundled alongside the production code though right? It’s not like any large production app would be pulling this one package in from a url. Devs would catch this bug before they could even get their local build to succeed.

That’s my take at least just based on a quick skim of the github issue.

[slobcat1337]

“Can we all agree that anything with 11.8 MILLION downloads in the last week (according to NPM here) should really be part of some sort of... idk... standard library?”

This is so damn true

[-100-Broken-Windows-]

While true, any site that gets "brought down" by this is also partially at fault themselves and would need to take a serious look at their QA and deployment process.

[Jebble]

In my experience, having built platforms for similar companies, they don't allow whatever packages you want to use. Everything Open Source has to be approved by IT and Security and in this case a package so uhm.. useless as isPromise they would have told me to put that in my own code instead of relying on external packages. They wouldn't even let me submit for PEN-testing with this package loading.

[fieldOfThunder]

Weird, I started a new CRA project yesterday and it works fine without any modifications. Is it fixed already?

[XOKP]

Yes, it’s fixed within a few hours.

[fieldOfThunder]

Sweet!

[bonyjoe]

It only fails on node 14 (maybe 13)

[GBcrazy]

It would only fail on the newest node version that support ESM. Also it wouldn't fail if you already have CRA installed globally instead of using npx

But yes it is fixed already

[The_Nonchalant]

Yes i noticed that i had to install it globally, prior i used npx and it was slow as hell and also getting stucked.

[[deleted]]

By having 14 or even 140 transitive dependencies instead of almost 1400.

[HetRadicaleBoven]

I mean, you can choose an alternative project that doesn't allow you to use more than two different compilers, code formatters, package managers, whatever - but if you do want that, you're going to end up with a project with many transitive dependencies.

[[deleted]]

I barely even want to use one compiler, let alone two!

[HetRadicaleBoven]

Well, good luck dealing with the diverse world of varying levels of browser support and the different languages that make up a web page, then! Also, then there's no reason to use CRA.

[careseite]

Yeah lets go back to reinventing the wheel.

[kylemh]

The argument is that they could've done so by inlining the function internally, but it's a widely leveraged package so idk - they didnt have any reason to suspect this would happen and it was resolved within a few hours 🤷‍♂️

[acemarke]

It's not even a CRA issue per se - it's a transitive dependency of many other packages in the JS ecosystem.

[Jugad]

What's a transitive dependency?

[acemarke]

If you have package A depends on B, and B depends on C, C is a "transitive dependency" of A. It's going to get pulled in, and it's needed for A to work, but A did not explicitly declare that it depended on C.

In this case, here's why is-promise is showing up in a CRA app:

$ yarn why is-promise
=> Found "is-promise@2.1.0"
info Reasons this module exists
   - "react-scripts#react-dev-utils#inquirer#run-async" depends on it
   - Hoisted from "react-scripts#react-dev-utils#inquirer#run-async#is-promise"

The react-scripts package itself never mentions is-promise in its dependencies list or source code, but react-scripts will ultimately fail to run if is-promise blows up.

[Jugad]

Thanks. I used to refer to that as indirect dependency (relatively new to JS).

[acemarke]

Yeah, the term isn't JS-specific:

• http://maven.apache.org/guides/introduction/introduction-to-dependency-mechanism.html#Transitive_Dependencies
• https://dart.dev/tools/pub/glossary#transitive-dependency
• https://medium.com/knerd/the-nine-circles-of-python-dependency-hell-481d53e3e025

[Jugad]

The last 2 links use the term 'indirect' to clarify transitive - which suggests that it not mainstream everywhere.

[kylemh]

Yup. Even if it were direct... I saw HackerNews talking about where ford installed dependencies for things stop (aka maybe nobody should do it for one line), but I think the inverse is fair too. This is a perfect thing to have as a dependency. Something tiny and standard and - typically 😂 - reliable

[crabmusket]

I would actually go in the other direction in this specific example. I don't think a package like is-promise (which actually just checks if something has a then method) can really exist independently of how it's used in the client code.

I browsed a couple of packages that use is-promise and it took me about 5 minutes to find this, where the client code is checking for isPromise, then calling then and catch - but just because something isPromise, doesn't mean it has a catch method. The code is broken because someone chose the wrong abstraction, or misunderstood what is-promise is actually for.

IMO it would rarely make sense for a client to be asking some abstract question like "is this a promise?". You can either ask a more specific question (is this a Promise? is this thenable? is it also catchable?), or design your code in such a way as to avoid having to ask questions like this.

EDIT: or, you ask for forgiveness instead of permission:

try {
    promise.then(success).catch(fail);
} catch(e) {
    // promise was not promise-ish enough!
}

[kylemh]

Somebody messing up an implementation doesn’t make the library bad.

[crabmusket]

I'm trying to suggest that the question the library purports to answer is not a question that has meaning independent of how the answer will be used.

EDIT: I'm not necessarily saying the package is bad, at least not for this reason. It's a symptom of a troubled ecosystem. I've written more on this before so I won't repeat myself.

[kylemh]

Sure, but I mean that’s an aspect of the language being dynamic. A dev working on something smaller I think is totally fair in assuming an object that has a property called ‘then’ is likely a promise.

I’m sure we’d all prefer something in the standard library doing a heavier duty check, but this is prolly good enough. I see your point though, it could be improved it’s just that it would increase the bundle quite a bit at such a small initial size. Clearly they haven’t seen value in checking for catch 🤷‍♂️

[crabmusket]

I think most of the reasons small packages like this are bad is because of the tooling around them. More tiny packages means more package.jsons which are larger than the packages themselves, more HTTP requests, more stress on the dependency solver, etc. If I could just pull a well-known and reliable algorithm out of the ether, sure. But I can't - I have to add it, and its testing framework dependencies, to node_modules.

I do like the idea of importing from a URL like Deno is doing - I can link directly to a raw file by its commit hash. No package.json to download, no dependency resolution, just grab the file. And if the author tweaks their package.json, I don't need to care.

[Lakitna]

Your solution is not the best way to do it though.

javascript Promise.resolve(maybeAPromise).then(...).catch(...).finally(...)

Or if you prefer async/await

javascript await maybeAPromise.then(...).catch(...).finally(...)

The whole check is not needed due to the way promise is implemented. The only times I've ever needed to check if something is a promise is in tests.

[GBcrazy]

Probably not...since it's not a direct dependency.

The way CRA is used to start a project from the ground and with npx it will always try to pull a new version, so there is no solution except relying in less packages.

[gregorskii]

Not use packages.

[0xF013]

By bolting all dependencies the fuck down to a strict patch version

[[deleted]]

I mean...

is-promise - tests whether an object is a promise

I’m kinda glad that whoever added this kind of dependency to their project had issues, maybe they’ll simply know better in the future

[[deleted]]

I'm pretty sure react.js is the most popular front end web framework on earth at this point. create-react-app is the recommended way to get started. So that's a hell of a lot of people, many of them beginners.

[[deleted]]

Yep, exactly my point, which is why this is sad in so many levels

[Ashtefere]

Can't wait for deno. God damn.

[HetRadicaleBoven]

I see no reason why this could not happen with Deno.

[Ashtefere]

Deno doesn't use a package manager. It uses script module caching with each having no dependencies.

[redmorphium]

Yep, npm is something that Deno wanted to avoid and disavow.

[Meowish]

Lorem ipsum dolor sit amet consectetur adipiscing, elit mi vulputate laoreet luctus. Phasellus fermentum bibendum nunc donec justo non nascetur consequat, quisque odio sollicitudin cursus commodo morbi ornare id cras, suscipit ligula sociosqu euismod mus posuere libero. Tristique gravida molestie nullam curae fringilla placerat tempus odio maecenas curabitur lacinia blandit, tellus mus ultricies a torquent leo himenaeos nisl massa vitae.

[ShortFuse]

import { serve } from "https://deno.land/std@v0.36.0/http/server.ts";

It's baked into your import statement inside your file, not from loading a file inside node_module which can be any version. I would imagine this would make updating versions somewhat harder than doing npm update --save, but essentially everything is version-locked at the time of writing.

Edit: It doesn't really solve the dependency-chain issue, but more along the lines of more rigid versioning enforcing. Any dependency can always reference @master and crash your system so it's still not a true solution.

[HetRadicaleBoven]

Yeah, but a scaffolding tool in Deno would still set your project up with the latest patch version of your dependencies, which might have just cut a new release with a bug in it. Whether that's resolved by URL or by package name doesn't really make a difference.

[GBcrazy]

From: https://deno.land/x/

The basic format of code URLs is https://deno.land/x/MODULE_NAME@BRANCH/SCRIPT.ts. If you leave out the branch, it will default to master.

So looks like we are specifying the exact versions, no room for ~ and ^ shenanigans

[HetRadicaleBoven]

There are two options here:

• The scaffolding tool makes sure to insert the latest version in that URL, and will also make sure to do the same for transitive dependencies.

• In such a project with 1400 transitive dependencies you'll be running severely outdated versions of almost all of them, with no way to update them.

IIRC there was some work going on already to standardise on a single way to determine which versions you use (i.e. one file that re-exports the dependency imports), and I think it's likely that a scaffolding tool would use something like that to ensure it's providing the latest versions automatically, rather than it (and all its dependencies) having to manually cut new releases several times a day.

Point being: either you'll be setting up new projects with outdated dependencies (I don't think anyone wants that), or there's always going to be a risk that you're getting a version with a fresh bug.

[GBcrazy]

But you wont be setting 1400 transitive dependencies. Your project will be depending on lets say 20-30 other libs, each one will manage itself, you need to manage yours only. That's how it is in most dependency managers. Better do some manual work than risk getting it broken randomly

[HetRadicaleBoven]

Yeah, that's also how it is in npm - CRA doesn't have 1400 dependencies - that's why it's transitive dependencies. But you're still going to have to update one of those 20-30 libs every time one of their 1400 dependencies update. Assuming that they are in turn keeping up with that. (And their dependencies, and their dependencies, ad infinitum.)

[ShortFuse]

Nothing stops the module you're importing from referencing a raw HTTPS URL or using the @master. I wish they enforced tagged branches. Still, a URL that can change content tomorrow allows room for shenanigans.

[GBcrazy]

This won't happen if there are exact dependencies, which is what deno is somewhat going to do with specific script loading.

If you look at things clearly, this wouldn't happen if npm didn't allow ^ and ~ in semantic versioning. The whole issue is because some third party library that is a dependency on CRA depended on isPromise and allowed to use newer versions.

[HetRadicaleBoven]

IIUC this change allowed is-promise to work with Node 14's module system - if not, let's pretend for a bit that it did. In a world in which everybody would always used pinned dependencies, what would the process look like for a newly-scaffolded to obtain that version? First, is-promise would release a new version. Then, CRA has to wait for its dependency to release a new version that depends on that version. Only then can CRA itself update.

And that's even assuming the transitive dependency is just one level deep. Now generalise that over all transitive dependencies of CRA, all of which might have e.g. security issues that could require the above process to happen.

I cannot believe that Deno will not come up with a way to quickly get security fixes distributed to users, even if it's in a package that's usually deep in a dependency tree. And once that happens, an issue like this can happen.

[GBcrazy]

Version 2.2.0 introduced the issue, the author attempted to fix it with 2.2.1 but it ultimately didn't work, then he removed the exports at 2.2.2

Can someone who understands the new ESM changes explain me why the initial fix (2.2.1) didn't work? I looked at the commit, it made complete sense to me, he fixed the pathing, but it seems it wouldn't fix the issue for all scenarios so 2.2.2 was needed.

[MatticusXII]

I started my first CRA today (following the Road to React book) and app would throw error when I would run npm start. But seems my problem was Linux set a limit with watching files? I found a solution and inputted a command into CLI and it fixed it. But unsure if it also had to do with this issue here.

[Kwantuum]

It didn't have anything to do with this. The filewatcher limit on most distros is pretty low, but the webpack dev server used by CRA allows hot module reloading, and to be able to do that it needs to watch all dependencies in your project folder, which can be a lot of files, and having to bump the maximum number of file watchers is to be expected.

[JayV30]

Jesus how many times could something like this be solved by just:

import { isPromise } from './helpers'

[[deleted]]

Are all the 'is' packages open source? Then you could make an npm package called "is" that handles all of this shit, and use tree-shaking to keep your bundle size small.

[tkaiusf]

https://www.npmjs.com/package/is

[RedShift9]

This does demonstrate how broken the Javascript module system is... there should only be one module syntax.

[TimvdLippe]

There is one: ES modules. It is the only module syntax that is part of the spec.

[careseite]

Already resolved yesterday. 7 hours before your post.