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

[FEATURE] Sapui5MavenSnapshotResolver: Use npm-dist.zip artifact for 1.116.0 and later #622

Merged
merged 5 commits into from
Jun 20, 2023

Conversation

RandomByte
Copy link
Member

@RandomByte RandomByte commented Jun 14, 2023

Use the SAP-internal npm-dist.zip instead of the default JAR when consuming SNAPSHOT versions.
This artifact also contains selected test-resources and will eventually be published to the public npm registry.

Since the extracted content is stored under the same directory path as the default JAR is stored in previous versions,
we now only consider a package to be installed when it has a package.json. This causes old jar based packages that already exist to be removed and replaced by the new npm-dist.zip contents.

JIRA: CPOUI5FOUNDATION-672

@coveralls
Copy link

coveralls commented Jun 14, 2023

Coverage Status

coverage: 95.846% (+0.07%) from 95.772% when pulling cb75240 on snapshot-use-npm-dist into 3de767f on main.

…nder different name from default JAR

This is to ensure compatibility with old UI5 Tooling versions that are
used in parallel. An old version might download the default JAR for
1.117.0-SNAPSHOT and store it under "<library name>-prebuilt".

While a new UI5 Tooling version would expect the content of the
npm-dist.zip artifact under that name.

Therefore, we use a new name for storing the new artifact.
@RandomByte RandomByte requested a review from a team June 16, 2023 13:37
Copy link
Member

@flovogt flovogt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but wait for others review

lib/ui5Framework/Sapui5MavenSnapshotResolver.js Outdated Show resolved Hide resolved
@d3xter666
Copy link
Contributor

LGTM

// Add "-prebuilt_dist" suffix to package name
// We can't use "-prebuilt" since old ui5-project versions might continue to download
// and store the default JAR under that name too
pkgName += "-prebuilt_dist";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether this is really the best solution. This is not the actual package name anymore.
Maybe the installer should rather have the knowledge on how to name the folders, because that's what is important here. The package name is actually not changing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we not just use the same name and ask for using the latest ui5-project?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also expect this to cause trouble. We need to use a different folder name. But only that and maybe the lock name need to differ.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Matthias and I found a potential solution to this. With the latest commit, we check whether the found "-prebuilt" package actually contains a package.json (which is only the case for the new npm-dist.zip artifact). In case no package.json is found, the installed package is overwritten with a newly installed npm-dist.zip package 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

matz3 and others added 3 commits June 19, 2023 16:23
with installed content from previous ui5-project versions.

The prebuilt package previously didn't contain a package.json.
Now we only consider a package to be installed when it has a package.json
This causes old jar based packages that already exist to be removed and
replaced by the new npm-dist.zip contents.
@RandomByte RandomByte changed the title [INTERNAL] Sapui5MavenSnapshotResolver: Use npm-dist.zip artifact for 1.116.0 and later [FEATURE] Sapui5MavenSnapshotResolver: Use npm-dist.zip artifact for 1.116.0 and later Jun 20, 2023
@RandomByte RandomByte merged commit 45dcee0 into main Jun 20, 2023
@RandomByte RandomByte deleted the snapshot-use-npm-dist branch June 20, 2023 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants