Better support for shipping method providers (single method, multiple prices / options)

[taxilian]

Is your feature request related to a problem? Please describe.

There are many shipping providers (e.g. shipstation, shippo, stamps.com, etc) which provide APIs which return an undetermined number of shipping methods with prices; currently in vendure a single Shipping Method can only return a single price, a single option.

Describe the solution you'd like There should be some method for a "ShippingMethod" to return multiple options and some way when selecting shipping to set not only which ShippingMethod but which of the options it provided to use.

On discord (https://discord.com/channels/1100672177260478564/1200213503139655750/1200340563724083290) the suggestion was proposed to allow a shipping calculator to return multiple prices in an attempt to make it a non-breaking change.

Describe alternatives you've considered

Two options I've thought about:

1. Shipping plugin provides custom APIs for getting shipping methods, adds custom fields to the order for which is selected. The ShippingCalculator then looks at those fields to decide if it's going to be used.

2. The shipping calculator return the available shipping options as metadata; again custom fields can be used to set which method to use but if not provided it can return some arbitrary (or arbitrarily chosen?) value.

Both are a bit hacky, I feel like 2 is hackier than 1 =]

Additional context

Here is an abbreviated example of what the Shipstation APIs might return. (not exact, but it's what my custom API returns after getting all available carriers and services)

[
    {
        name: 'Stamps.com',
        code: 'stamps_com',
        nickname: 'USPS / Stamps',
        shippingProviderId: 54321,
        primary: true,
        services: [
            {
                serviceName: 'USPS First Class Mail - Package',
                serviceCode: 'usps_first_class_mail',
                shipmentCost: 3.79,
                otherCost: 0,
            },
            {
                serviceName: 'USPS Priority Mail - Package',
                serviceCode: 'usps_priority_mail',
                shipmentCost: 6.64,
                otherCost: 0,
            },
        ],
    },
    {
        name: 'UPS by ShipStation',
        code: 'ups_walleted',
        nickname: 'UPS',
        shippingProviderId: 12345,
        primary: true,
        services: [
            {
                serviceName: 'UPS Next Day Air®',
                serviceCode: 'ups_next_day_air',
                shipmentCost: 17.76,
                otherCost: 0,
            },
            {
                serviceName: 'UPS® Ground',
                serviceCode: 'ups_ground',
                shipmentCost: 6.76,
                otherCost: 0,
            },
        ],
    },
]

[jacobfrantz1]

A somewhat related issue is error handling in shipping calculators. If an api call fails, it either has to throw which fails the entire getEligibleShippingMethods query, or return a fallback price; neither of which are great.

[brunoslalmeida]

Hi @michaelbromley ,

I'm diving into this feature request and looking forward to coding it up. Given its importance for my e-commerce platform, I wanted to double-check before starting my experiments. Are there any specific areas or functions I should be particularly careful not to modify?

[brunoslalmeida]

@michaelbromley i've added the code here github it would be awesome to get some insights from you before moving forward.

[brunoslalmeida]

[michaelbromley]

Hi @brunoslalmeida I just took a look through your fork.

I think the overall direction looks good. I have some feedback on the API changes:

export const TEST_SHIPPING_METHOD = gql`
    query TestShippingMethod($input: TestShippingMethodInput!) {
        testShippingMethod(input: $input) {
            eligible
            quote {
                price
                priceWithTax
                metadata
            }
            list {
                eligible
                quote {
                    price
                    priceWithTax
                    metadata
                }
            }
        }
    }
`;

• In the list array, the eligible property is redundant as far as I understand? It is based on the eligibility checker which would will be either true or false and will apply to all computed price quotes.

I would prefer this:

    query TestShippingMethod($input: TestShippingMethodInput!) {
        testShippingMethod(input: $input) {
            eligible
            quote {
                price
                priceWithTax
                metadata
            }
            quotes {
                price
                priceWithTax
                metadata
            }
        }
    }

and we can deprecate the quote field for removal in a future version, and internally we just always use the quotes array.

[brunoslalmeida]

Yes, it is redundant it was already in plans to remove it. I was also struggling to find a name, quotes seams like a good name. I will fix it and continue.

[brunoslalmeida]

Hey @michaelbromley just finished your feedback changes (i was struggling with time) and will get the pace back this week. Thanks for your support.

[brunoslalmeida]

Hi @michaelbromley,

I have a question.

As far as I'm studying, the entry point for shipping selection at shop-api is the mutation setOrderShippingMethod. This mutation only receives a list of ShippingMethod IDs. However, knowing just the ID isn't enough to select the inner choice. I'm struggling to think of a solution that maintains backward compatibility.

My only current option is to create a new optional argument that receives an array with the same size as the first one containing IDs to identify the inner ID. If no information is sent by the front-end, then the first item of each method should be selected. To achieve this, an optional ID needs to be added to ShippingCalculationResult.

For admin-api the entry-point is setDraftOrderShippingMethod. A new optional arg can be added and in case of null use first. This is easier.

What are your thoughts on this? Am i missing something important ?

[brunoslalmeida]

I am also testing with remix, and the front-end is considering the same item (probably due to the same method id). Is this a retro-compatibility problem? Or if at the end a new version of the front end gets generated is not a problem ?

[michaelbromley]

Hi,

regarding the setOrderShippingMethod mutation, a non-breaking change could be to:

1. make the shippingMethodId input nullable (remove the !)
2. create a new input which takes an array of objects, whose shape you can decide based on the data you need.

Once we make changes to the API, we can update the storefront implementations as needed.

[kbanman]

I'm also looking for this functionality and can contribute to the implementation if there's a need, just let me know where I could be helpful.

I agree with the backward-compatible change suggested above. Don't know if there is an established pattern for deprecation but it would be good to discourage the use of the old signature after release so as to enable removing it at some point.

[kbanman]

@brunoslalmeida a small comment on your changes:

Instead of having checkEligibilityByShippingMethod return either a single or array, normalize the results to just returning an array. Since it's a private method we don't need to make it backward compatible. This saves on conditionals elsewhere.

Same goes for ShippingMethod.apply, simplify from Promise<ShippingCalculationResult | ShippingCalculationResult[] | undefined> to Promise<ShippingCalculationResult[]>

[brunoslalmeida]

Hey @kbanman we are facing a possible breaking change at other places. I've discuss with @michaelbromley at discord and will bring it here. The problem is that for on shippingMethod with multiple results there must be a new field (this is the db breaking) to select witch item from the results is being selected.

[brunoslalmeida]

This is the discussion thread. https://discord.com/channels/1100672177260478564/1218990088403288074

[brunoslalmeida]

@brunoslalmeida a small comment on your changes:

Instead of having checkEligibilityByShippingMethod return either a single or array, normalize the results to just returning an array. Since it's a private method we don't need to make it backward compatible. This saves on conditionals elsewhere.

Same goes for ShippingMethod.apply, simplify from Promise<ShippingCalculationResult | ShippingCalculationResult[] | undefined> to Promise<ShippingCalculationResult[]>

@kbanman I do also think that retrocompatibility here might not be necessary. As soon as we handle the breaking problem with @michaelbromley I will test this change.

[michaelbromley]

So one way we could handle this in a non-breaking way would be to put it behind a feature flag like

shippingOptions: {
  multipleQuotesPerShippingMethod: true
}

and depending on that setting, we can dynamically define the field in the DB and extend the GraphQL API. In general I'm not a big fan of adding feature flags like this, because they complicate the code base in multiple places. But I'm not sure if there's a better solution without breaking things.

An alternative is to leave this for a future major release.

[brunoslalmeida]

The ideal solution would be to leave this for future major. I am currently planning to create a custom base plugin that allows developers to create their own solution with its supplier as a temporary solution while this is not solved via major. This custom base plugin will have a function base param so the developer can create it's own calls to the supplier and the plugin will handle the multiple shipping function using custom fields to add the selected method to the order.

[brunoslalmeida]

@michaelbromley I am moving back to this issue and would like your help with the feature flag and entity. I am a bit confused how to validade the feature flag at the entity file in order to just allow the new field creation if the parameter is true, can you give-me some light ?

[brunoslalmeida]

I am also thinking that it might be necessary to add validation if the calculator is returning an array and the multipleQuotesPerShippingMethod is false and notify user that this is problem. @michaelbromley Is this ok for you?

[michaelbromley]

Since entity properties (columns) are defined statically as class members with decorators, we'd need to take a different approach to add a property at run-time based on a flag. Luckily we do have a mechanism for this: EntityMetadataModifier.

So the idea would be to define a modifier function that adds the desired column to the entity during bootstrap. This happens here for user-defined modifiers. You could create one and run it directly after that in the case that the feature flag is set on.

Since this is adding a bit of complexity, try to think of ways to keep all flag-related code organized in a way that is clear and will be easy to refactor when we come to the next major and want to make it the default.

I am also thinking that it might be necessary to add validation if the calculator is returning an array and the multipleQuotesPerShippingMethod is false and notify user that this is problem.

Yes that sounds reasonable.

[brunoslalmeida]

Ok, thanks I will take a look and do homework.

[brunoslalmeida]

@michaelbromley help clarify some doubts.

commit

1. Adding metadataModifiers does not trigger the message to create migration, is this the correct behaviour or did I do something wrong? Running migration creates the field correctly.

2. Is at bootstrap the correct place to add the function ?

3. Any idea for the field name, I am using code just for tests now

Do you have any other input ?

[michaelbromley]

Adding metadataModifiers does not trigger the message to create migration, is this the correct behaviour or did I do something wrong? Running migration creates the field correctly.

The migration message "Your database schema does not match your current configuration..." is not actually part of the bootstrap process. It is shown in a standard installation because by default we run the runMigrations function prior to bootstrap which in turn calls checkMigrationStatus

Is at bootstrap the correct place to add the function ?

Yes - the metadata modifier needs to be executed at that point.

Any idea for the field name, I am using code just for tests now

This is a unique identifier for the given quote from a single ShippingMethod right? I think code is fine.

Do you have any other input ?

This is out-of-scope for this specific issue, but the code makes me think we need a proper framework for implementing feature flags in a cleaner way. Right now we would have feature-flag code scattered around the code base. For one or two feature flags this is tolerable (if ugly) but the tendency is always towards more chaos :D so maybe it is a good time now to think about what the right way would be to handle this.

Imagine something like this: we define a FeatureFlag interface which allows us to make the process a bit more consistent:

abstract class FeatureFlag {
  name: string;
  abstract determineEnabledStatus(config: RuntimeVendureConfig): boolean;
  protected _isEnabled: boolean;

  get isEnabled() {
    return this._isEnabled;
  }

  constructor(config: RuntimeVendureConfig) {
    this._isEnabled = this.determineEnabledStatus(config);
  }
}

and then we define the flag like this:

// packages/core/src/feature-flags/multiple-shipping-quotes.ts
export class MultipleShippingQuotesFeatureFlag extends FeatureFlag {
  name: 'Multiple shipping quotes',
  determineEnabledStatus(config) => config.shippingOptions.multipleQuotesPerShippingMethod === true,

  // other logic/methods can be defined as methods 
  //on the feature flag class to better encapsulate things
  addShippingMethodCode(config: VendureConfig) {
    if (!config.entityOptions.metadataModifiers) {
      config.entityOptions.metadataModifiers = [];
    }

    const addShippingMethodCode: EntityMetadataModifier = metadata => {
      const instance = new ShippingLine();
      Column({ type: 'text', nullable: true })(instance, 'code');
    };
    config.entityOptions.metadataModifiers.push(addShippingMethodCode);
  }
}

At some point in the bootstrap process we instantiate all the feature flags:

// packages/core/src/feature-flags/index.ts
export const FEATURE_FLAG = {

};

// bootstrap.ts
FEATURE_FLAG.multipleShippingQuotes = new MultipleShippingQuotesFeatureFlag(config);

and use it like this:

// bootstrap.ts

if (FEATURE_FLAG.multipleShipping.isEnabled) {
  multipleShippingQuotesFeatureFlag.addShippingMethodCode(config);
}

Note: I don't expect you to implement this. I just wanted to jot down my idea as it came to me. For now you can continue development and it should be easy to refactor to use whatever feature flag system we implement in the future.