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

Flags like --unstable Should Not Apply To Every Library #7947

Closed
Skillz4Killz opened this issue Oct 12, 2020 · 9 comments · Fixed by #8050
Closed

Flags like --unstable Should Not Apply To Every Library #7947

Skillz4Killz opened this issue Oct 12, 2020 · 9 comments · Fixed by #8050
Labels
suggestion suggestions for new features (yet to be agreed)

Comments

@Skillz4Killz
Copy link

Skillz4Killz commented Oct 12, 2020

#7800 I just saw this now from a PR removing this in vscode plugin and am speechless. The amount of time I wasted editing hundreds of files on 5 different projects because this option was enforced because one of the deps that my modules user needed had unstable that had nothing to do with my modules at all 😭 Really disheartening to see all that wasted time and now needing to undo it all.

Applying a single flag should never affect every library. Just because the user of my lib has --unstable for another library, Deno marks all libs as using --unstable and causing users to see my lib as unstable/broken and preventing them from running their project. I don't believe this is a good solution to apply the flag to everything. Even though my lib is not unstable, my lib is now the reason they can not run their project, so it's not an option but forcing me to update all my code and then in under a month later "Whoops, we don't need this anymore."

@lucacasonato
Copy link
Member

now needing to undo it all

You don't need to undo it - all code that worked with the importsNotUsedAsValues: "error" also works without. Also no-one is forcing you into supporting --unstable.

Applying a single flag should never affect every library

Yes it should. Permissions? All modules. --no-check? All modules. --v8-flags? All modules. --config? All modules. --importmap? All modules. The logical consequence is that --unstable also applies to all modules.

"Whoops, we don't need this anymore."

That is the point of --unstable... We use it to test things. If you don't like that, don't use it, and don't support your users using it. It is called unstable for a reason - it is subject to change without prior warning.

@Skillz4Killz
Copy link
Author

Skillz4Killz commented Oct 12, 2020

You don't need to undo it

Code was made in my opinion to be much uglier so yes I will undo it. Maybe not as much as a rush as before, but for sure that ugly code should be cleaned up.

Also no-one is forcing you into supporting --unstable

When Deno is telling my libs users that my lib is having errors and their projects can't run until my lib is fixed yes it is forcing. This isn't an option. Deno should not be applying --unstable to stable libraries.

Permissions

There is already a full discussion about this and even Ry has stated he is very interested in doing this where Permissions can be separated but he doesn't know it is possible. Someone needs to do the research first. But it is something he would want to do.

#171
https://www.youtube.com/watch?v=r5F6dekUmdE#t=1h31

--no-check

Yes i think this should also not apply to all code. If 1 library is having an issue preventing ur project from running you could no-check that library, not all your code and every other deps code as well that works perfectly without no-check.

--v8-flags

I'm not sure about this but i think this would merit running on all code.

--config

This is 100% something that should not run in each library. I opened an issue about this separately which sparked the proposal of removing the entire --config flag from Deno. #7701 #7732

--importmap

This is something that still doesn't work properly (if i am not mistaken), as you can't include this in libraries only end user code. So not really sure how this matters if this flag effects libs.

--unstable

The fact is that this flag is causing Deno to cause problems in stable libraries that DO NOT use --unstable. Using --unstable as a test needs to stop so long as these tests also end up effecting stable code. A better solution needs to be made that does not effect stable code. Deno is no longer stable if it breaks stable library code.

I can't exactly say hey you need MongoDB in your project sorry I won't allow you to use my lib because Deno told me not to allow you to use my lib if you want MongoDB.

--unstable isn't exactly a good testing ground when you make the changes in unstable effect everything that is stable also.

@andreespirela
Copy link

I believe the impact you're experiencing is because when you use --unstable that sets isolatedModules to true. And that causes a downstream effect which I believe is what forces libraries to use import type, export type, is this right? @lucacasonato

@nayeemrmn
Copy link
Collaborator

nayeemrmn commented Oct 12, 2020

Regardless of your experience, this is impossible. The --unstable feature set applies to the runtime API which is underneath all libraries imported into a project. By the way, this also has nothing to do with --unstable as the feature was on-track to be stabilised next minor version.

The fact is that this flag is causing Deno to cause problems in stable libraries that DO NOT use --unstable.

Your library does use --unstable, by means of depending on something else which uses an unstable feature.

@Skillz4Killz
Copy link
Author

Skillz4Killz commented Oct 12, 2020

@andreespirela Correct.

The --unstable feature set applies to the runtime API which is underneath all libraries imported into a project.

@nayeemrmn Then perhaps this entire concept needs to be rethought of using the unstable flag as a testing ground for testing potentially breaking changes to even stable projects. If you can't control the effects of unstable on stable projects, then this isn't a good way to implement massive breaking changes. This change should then have been delayed to 2.0 or a method needs to be added to allow controlling this runtime API on a per-module basis.

Your library does use --unstable, by means of depending on something else which uses an unstable feature.

@nayeemrmn Incorrect. My library has no dependencies on anything using unstable. The user who uses my library added a dependency, in this case, MongoDB for their project which has 0 effects on my lib but MongoDB requires unstable so now every lib is effected. In essence, Deno is causing problems in stable libraries that DO NOT use --unstable

Also of note is that the --unstable flag is not the only one that has issues like this, it is just the one bringing the main issue to light center stage. Other flags like this as I mentioned earlier have similar problems when they effect the entire codebase like configs flag and permission flags.

TLDR: The problem isn't 1 flag in general but the concept that a flag is enforced on every library.

@andreespirela
Copy link

andreespirela commented Oct 12, 2020

@Skillz4Killz I understand the concern, I myself had to refactor a lot of code for one of my packages to work the same way I did when 1.2 came out which broke many packages. But I have to say, I don't think this is a Deno issue, there are several things to address:

  1. As mentioned here --config should not apply to deps #7701 , as

@kitsonk "I don't think we want to support this. The complexity versus value is too great, even if it was achievable."
Also mentioned by
@nayeemrmn "The --unstable feature set applies to the runtime API which is underneath all libraries imported into a project."

(While @kitsonk referred to TS configuration, afaik the same logic applies here, it is something handled by and to the runtime API, not something of "module individualism")

The complexity of it can even make it an impossible feature to have, flags such as --config and --unstable affect the way the typescript compiler behaves, for the typescript transpiler, there is no such thing as "external module" or "libraries from Deno.land/x", there is just a bunch of code it will process based on your deno command.

  1. Technically, the export type; import type wasn't Deno's fault , it was more of a breaking change in Typescript which leads to the following points:
    • It is of people using Deno to maintain their packages up-to-date. Yes, that includes maintaining it with the latest typescript versions.
    • It is of people using Deno to maintain their compatibility with Deno's latest versions.

If a code is well maintained, --unstable will not have any impact on stable libraries, and vice versa.

That's my take on it. It is hard for me to say all that because I am an stubborn person when it comes to breaking changes, but I'm not really sure if there's a point to be made about the --unstable flag.

@Skillz4Killz
Copy link
Author

Skillz4Killz commented Oct 12, 2020

If a code is well maintained, --unstable will not have any impact on stable libraries, and vice versa.

@andreespirela How do you explain every library breaking that doesn't use --unstable? My library broke and it caused me to have to fix hundreds of files. The fact that another library can cause my library to be unusable is crazy. It is properly maintained and I try to keep it updated to TS changes all the time. In fact, I am even trying to be ahead of the curve and fixing for things in 4.1 which is what sparked #7701

Note: I am not blaming Deno for the whole export-import type stuff. My concern is with the fact that another library can break my library.

If a user adds a dependency that requires them to have --unstable, suddenly every library in existence is treated as unstable even though they are perfectly stable and do not expect to be broken. As a lib maintainer, it is an absolute nightmare to have to always keep libs up to date with unstable. Users that add the --unstable flag aren't being told that the problem lies in the unstable dependency they are being told the problem is from my library(which is stable). How is every library maintainer expected to keep up to date with unstable changes?

I can't just simply tell the entire community, "If you want to use Deno, don't use MongoDB as almost every Deno library will break once you add MongoDB since it uses --unstable." Or maybe "Stop using TypeScript as Deno doesn't play well with TS compiler/configs causing unstable libraries to break other stable libraries." These are not valid at all. TS is amazing and so is MongoDB. My library should not break because a user decided to use TS and MongoDB.

I don't know how complex it is to achieve this, I really don't. But what I do know is how problematic it is to maintain a library in Deno because of this and I would argue it is much more harmful for the long term survival of Deno community if the libraries are impossible to maintain as they require always needing to be up to date with unstable flag.

Deno unstable flag should never break production ready stable libraries or at the least it needs to clarify very well that errors are happening BECAUSE of the unstable flag and the developer should remove the --unstable. The problem isn't in the stable production ready libraries but the unstable libraries.

@kitsonk
Copy link
Contributor

kitsonk commented Oct 12, 2020

Woah. This is a lot of words...

First we have a point in time problem. We allowed a lot of TypeScript in Deno that causes problems with --no-check. Being able to type strip important feature for everyone. We needed a way to try to get folks to update their code in advance of us making it on by default. Being able to run code in Deno that works both with and without --no-check without errors is good for everyone. If we could go back in time, we would have forced that from the start, but our time machines are broken.

I think given the maturity of Deno, everyone should expect that we might break things and people will have to update their code. We will try to be mindful, but dragging around lots of legacy code is something that doesn't benefit anyone.

Having 20 different versions of runtime environments with 20 different config options and a complex configuration for the end user to say "this module runs like this and this module runs like that" is also not a good thing. That means sometimes we will break things though. Part of the reason why we put these things behind --unstable. The fact that people are shipping "production" libs that require --unstable is an indication that no matter what we do people will still try to be on the bleeding edge and pursued others to unwittingly join them.

I am hoping with 1.5 we settle down the fundamentals of what "flavour" of TypeScript we run in Deno along with a good set of documentation of why things are the way they are. This will though break some code, but having a fixed position that people can reliably develop for and have confidence that their end users can use in Deno reliably.

We could have tried to make these changes with another flag, but no one would have used it and less code would have been updated. Dammed if we do damned if we don't.

@Skillz4Killz
Copy link
Author

Skillz4Killz commented Oct 12, 2020

We could have tried to make these changes with another flag, but no one would have used it and less code would have been updated

I did use it only for all that time I spent updating my lib for it to have been wasted and I have to, unfortunately, undo it all because it's been undone. Perhaps this should have been for 2.0 which would have prevented me from doing this unnecessarily.

@kitsonk I agree wholeheartedly with you about almost everything you said. The one thing I slightly disagree with is that 1.5 will solve this issue. The problem isn't TS itself but the fact that --unstable is applied to all deps even those that are stable. Even if Deno 1.5 is released and TS defaults are complete we will still have issues.

Important Points:

  • --unstable is being used as a testing ground.
  • Users that use --unstable end up breaking all libs INCLUDING stable libs that don't use unstable because the changes Deno is testing in --unstable is not supported yet.
  • Users are told the bug/crash/error is from the stable lib so users come to me complaining my lib is not letting their project to work. In reality, the issue isn't my lib which is stable.

I see three potential solutions which I have stated above:

  1. --unstable should not apply to every lib.
  2. --unstable can not be used as a testing ground for Deno's breaking changes that can break stable code. For example, if Deno is adding a new feature that does not breaking existing stable libs but is not fully finalized it can be tested in unstable. The major breaking changes must be done/tested in a separate way such as 2.0.
  3. When users use --unstable it should tell the users the errors are because they using the unstable dependency and they should remove it, Deno should not tell the users that the errors are from a stable library because it doesn't meet the changes Deno is testing in unstable.

As an aside, I also disagree with The fact that people are shipping "production" libs that require --unstable....

Users are opting to use "unstable" libs knowing they are unstable because there are not any stable alternatives. I can't stop them from using unstable libs. I can't stop them from using unstable std features. I can't stop them from using unstable libs because they need MongoDB. Only thing we can do is handle --unstable better.

@kitsonk kitsonk added the suggestion suggestions for new features (yet to be agreed) label Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
suggestion suggestions for new features (yet to be agreed)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants