Fix: replace global variable and refactor code

[zongqichen]

No description provided.

[aramovic79]

I've reported a few findings.
cc: @zongqichen @Fannon @swennemers

[aramovic79]

Putting two cents:

enums vs objects in javascript

You can find on many places that javascript developers prefer using Object.freeze instead of enum (or as const casting).

Enum can be defined as:

    const RESOURCE_TYPE = Object.freeze(
    {
        "api": "api",
        "event": "event"
    });

Please pay attention on Object.freeze. It serves to disallow changing of the object which at the end causes that RESOURCE_TYPE acts as an enum.

[aramovic79]

I think that the code:

function _getPackageID(namespace, packageIds, apiOrEvent) {
    if (packageIds instanceof Set) {
        const packageArray = Array.from(packageIds);

        if (apiOrEvent === "api") {
            const apiPackage = packageArray.find((pkg) => pkg.includes("-api"));
            if (apiPackage) return apiPackage;
        } else if (apiOrEvent === "event") {
            const eventPackage = packageArray.find((pkg) => pkg.includes("-event"));
            if (eventPackage) return eventPackage;
        }

        return packageArray.find(pkg => pkg.includes(namespace));
    }
}

can be written this way:

function _getPackageID(namespace, packageIds, apiOrEvent) {
    return packageIds.find((pkg) => pkg.includes("-" + apiOrEvent)) || packageIds.find((pkg) => pkg.includes(namespace));
}

where I think that instead of apiOrEvent we should name this input parameter like resourceType

[zongqichen]

I think that the code:

function _getPackageID(namespace, packageIds, apiOrEvent) {
    if (packageIds instanceof Set) {
        const packageArray = Array.from(packageIds);

        if (apiOrEvent === "api") {
            const apiPackage = packageArray.find((pkg) => pkg.includes("-api"));
            if (apiPackage) return apiPackage;
        } else if (apiOrEvent === "event") {
            const eventPackage = packageArray.find((pkg) => pkg.includes("-event"));
            if (eventPackage) return eventPackage;
        }

        return packageArray.find(pkg => pkg.includes(namespace));
    }
}

can be written this way:

function _getPackageID(namespace, packageIds, apiOrEvent) {
    return packageIds.find((pkg) => pkg.includes("-" + apiOrEvent)) || packageIds.find((pkg) => pkg.includes(namespace));
}

where I think that instead of apiOrEvent we should name this input parameter like resourceType

I create an issue regarding to this refactor: #73

[aramovic79]

I would name this constant as "CDS_COMPILE_TO" and also add the other targets: sql, hana, asyncapi, json, hana, edm, yaml and cdl. Just for the sake of completeness.

[aramovic79]

Suggested change

	const CONTENT_MERGE_KEY = 'ordId'
	const ORD_KEY = 'ordId'

[aramovic79]

I don't understand why is somewhere called imported function(like asyncapi here), and somewhere is used the whole cds.compile.to... statement. Is there some particular reason?

[aramovic79]

This is a private function, right? Maybe to name it _generatePath?
Also, when defining functions, on some places we use function and on the others const. Do we follow some rule when to use what? I couldn't find any regularity, it seems it was randomly/arbitrarily decided.

[zongqichen]

As #57 discussed, we should use function

[aramovic79]

We are calling this function in multiple places in a sequence when generating ord document and every time we have the same input parameter - serviceDefinition. Why not to call this function once when starting generating ORD document and then pass the result as an input parameter of all subsequent functions: _getGroups, _getApiResources, _getEventResources... ?

[aramovic79]

[swennemers]

let's close this PR and fix this as part of the next PR #64