feat: add skipValidation to dart publishing

[vaind]

This will be necessary for a while because we keep the new dart core package package:web dependency implicit in order to keep the pacakge compatible with old Dart/Flutter SDK users. Some context in getsentry/sentry-dart#2113

[kahest]

@getsentry/dev-infra this currently blocks our https://github.com/getsentry/sentry-dart releases, so we'd greatly appreciate your help getting this merged 🙏

[asottile-sentry]

I don't think we should add this option. validation is there to prevent bugs and it feels like you should fix those instead of bypassing them to ensure a quality release

[vaind]

We have this validation in the repository itself where the decision was already made to release regardless of the reported issue (after it was tested and seems to work in the scenarios).

[asottile-sentry]

I don't see any of that in the linked discussion -- did you link the correct issue?

[vaind]

I don't see any of that in the linked discussion -- did you link the correct issue?

How is that relevant for this PR? The decision has been made in the SDK itself. Craft is a tool to facilitate releases. If the SDK maintainers have decided they need to pass an argument to the dart sdk tooling, Craft should support it. Reviewing the decision to make the release regardless of warnings should not be part of this PR IMO

[asottile-sentry]

craft should be enforcing best practices.

imo turning off safety checks at a whim should not be a core feature

[bruno-garcia]

We have this validation in the repository itself where the decision was already made to release regardless of the reported issue (after it was tested and seems to work in the scenarios).

This is a reasonable explanation to why craft should expose a flag that exists in the underlying program it invokes.
Sounds like we're bikeshedding this change

[BYK]

Craft is an open-source tool available and used by outsiders. I think @asottile-sentry's request for clarification on the consequences of skipping validation and why this change is needed is quite well-founded.

Currently, without this context, I have no idea why this change is being made, why it is useful etc. This is needed for maintainers and contributors to this repo to understand and maintain the codebase.

[BYK]

FYI, I have no formal power in this repo but if I did, I'd have blocked the change until the reasoning was clearly stated and recorded.

[buenaflor]

Yeah I completely understand your side, from an outsider perspective without context adding a flag that ignores executed validations doesn't seem to be a good idea.

I'll try to explain it without going too much into detail but if you need more in-depth details then this PR serves as a good start: getsentry/sentry-dart#2113

So Flutter has recently stabilized their WASM support which turns out is not compatible with our SDK anymore because we're supporting an older min version that use older API (dart:html).

The first solution to that problem would be to migrate from the packages dart:html to package:web (their new API) which in theory sounds great and is also recommended but package:web expects a much much higher min version of Dart and Flutter (which are only a couple months old) and bumping our minimum versions to match that is not an option for us since we care about backwards compatibility a lot.

The point behind all of this is we want to be able to allow WASM compilation while maintaining backwards compatibility with our current minimum Dart and Flutter versions.

So @vaind created a working solution that implicitly uses the package:web dependency without us having to add the package:web dependency into our repo. (that's a simplified explanation btw) so of course that is a workaround, ideally we could just bump the versions and just ingest that dependency but we can't and won't do that.

So now that we have a working solution we're getting validation errors from the analyzer during publish because it's not typically implemented that way, after all it's a workaround, but it's so far the only good solution we found to that problem. We've already confirmed that it works and have tests in place in the SDK.

Hopefully that clears it up a bit 🙏

[kevmoo]

I think this is just a stop-gap to handle a temporary work-around. We have an important customer we'd love to unblock with this fix. 🙏

[vaind]

Craft is an open-source tool available and used by outsiders. I think @asottile-sentry's request for clarification on the consequences of skipping validation and why this change is needed is quite well-founded.

Currently, without this context, I have no idea why this change is being made, why it is useful etc. This is needed for maintainers and contributors to this repo to understand and maintain the codebase.

This PR doesn't skip validation. It exposes a dart tool option so that it can be used by users of craft, if they decided to do so. Have a look at the test code:

  test('should set default options', () => {
    const target = createPubDevTarget();
    expect(target.pubDevConfig).toStrictEqual({
      PUBDEV_ACCESS_TOKEN: DEFAULT_OPTION_VALUE,
      PUBDEV_REFRESH_TOKEN: DEFAULT_OPTION_VALUE,
      dartCliPath: 'dart',
      packages: ['.'],
      skipValidation: false,
    });
  });

  test('should allow for overwriting default options', () => {
    process.env.PUBDEV_ACCESS_TOKEN = 'access';
    process.env.PUBDEV_REFRESH_TOKEN = 'refresh';
    const target = createPubDevTarget({
      dartCliPath: '/custom/path/dart',
      // GH Actions .yml format
      packages: {
        uno: undefined,
        dos: undefined,
        tres: undefined,
      },
      skipValidation: true
    });

    expect(target.pubDevConfig).toStrictEqual({
      PUBDEV_ACCESS_TOKEN: 'access',
      PUBDEV_REFRESH_TOKEN: 'refresh',
      dartCliPath: '/custom/path/dart',
      packages: ['uno', 'dos', 'tres'],
      skipValidation: true,
    });
  });

It doesn't sit right with me that we're discussing here in the Craft repo PR whether the decision to use that option was made correctly. That is a discussion that belongs to the Dart SDK, whether it is a new issue or one of the existing PRs and should be taken with Dart SDK maintainers and other interested parties.

On the other hand, technically, dart tool (pubdev target) also seems to be the exception among craft targets, as most (if any) don't seem to run tests or analysis as part of the package publishing. That is the responsibility of the SDK relying on craft to make its releases.

[bruno-garcia]

Craft is an open-source tool available and used by outsiders. I think @asottile-sentry's request for clarification on the consequences of skipping validation and why this change is needed is quite well-founded.

I agree asking for clarification is well-founded and I appologize if my comment seem to address this point. It's completely fair to try to understand why we need to add a flag to skip validation as intuition will tell us that's a bad idea.
In my view, reading the last comments and following the links (PRs and issues in the Dart repo) it makes sense to go ahead with this change now.

FYI, I have no formal power in this repo but if I did, I'd have blocked the change until the reasoning was clearly stated and recorded.

After this message @buenaflor @kevmoo and @vaind chimed in with further clarification. Hopefully that helps shed light into the situation and why we need this flag exposed.

[dbennett-sentry]

After discussing with several people it appears as though we have a few things happening:

• We're trying to use Craft to raise a quality bar through enforcing validations.
• This enforcement is stronger in Dart than other platforms (a good thing) and is creating an obstacle to accomplishing real-world needs (not a good thing).
• Effort has been made to work-around the situation but most everyone has come to the same conclusion: we're in a pickle created by Craft itself -- even if for good reasons.

Therefore, let's move forward with this:

• Expose the flag as this PR does.
• Update the documentation to reflect real use case and severity of the flag. I.e., *optional* doesn't make it clear when, how, why, and why not to use skipValidation and it feels like a pretty big hammer without guidance.
• Once the documentation points to some use-cases and the missing context I'll approve this PR myself.

Following this PR, we'll work toward a standard for this sort of thing. Craft needs to work with platform peculiarities in a harmonious way and I don't think it's there yet.

[dbennett-sentry]

Thank you for the additional context in the documentation! Please ensure it's added before you merge.

[vaind]

Thank you for the additional context in the documentation! Please ensure it's added before you merge.

Thanks, much appreciated. I've applied the suggestion.

I cannot merge, though, because I'm not added to this repo.

[bruno-garcia]

Thanks @dbennett-sentry

[asottile-sentry]

this PR breaks the readme rendering