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

Web components: Introduce versioning #574

Merged
merged 6 commits into from
Sep 12, 2023
Merged

Web components: Introduce versioning #574

merged 6 commits into from
Sep 12, 2023

Conversation

tkohr
Copy link
Collaborator

@tkohr tkohr commented Aug 11, 2023

The goal of this PR is to introduce versioning to gn-ui web components. It should publish the gn-wc.js script to a wc-dist-main branch upon merges on main and to wc-dist-[tagname] branches when creating version tags complying to 'v*.*.*'.

Potential to-dos:

  • There might be an additional step necessary to create the publish_branch, if it does not exist yet.
  • The data-view-web-component.component does not use the correct version of the web component script yet. Not sure how to solve this best. => solved via new approach
  • Adapt/fix tests

@github-actions
Copy link
Contributor

github-actions bot commented Aug 11, 2023

Affected libs: feature-record, feature-router,
Affected apps: datahub, webcomponents, metadata-editor, demo,

  • 🚀 Build and deploy storybook and demo on GitHub Pages
  • 📦 Build and push affected docker images

@coveralls
Copy link

Coverage Status

coverage: 84.075% (-2.9%) from 86.95% when pulling d64d4f6 on web-components-versioning into 7503133 on main.

Copy link
Collaborator

@jahow jahow left a comment

Choose a reason for hiding this comment

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

Thanks! I feel like this currently brings a lot of complexity in the docker build process just for the WC embedder to use the correct gn-wc.js version.

Instead maybe we could somehow make the current gn-ui version available in the angular app. Then the version could be used in the data-view component both for the share url and the web component snippet.

Also the wc-embedder.html script could take the version of the gn-wc.js as an URL argument? but that could hurt the loading time significantly, not sure about that.

@jahow jahow added this to the 2.0.0 milestone Sep 6, 2023
@tkohr tkohr force-pushed the web-components-versioning branch from d64d4f6 to fb81e97 Compare September 8, 2023 14:22
@tkohr
Copy link
Collaborator Author

tkohr commented Sep 8, 2023

Thanks for the feedback @jahow ! I've rebased and reworked the approach how to use the fitting version of the web component within the datahub components as well as the wc-embedder following your suggestion. This seems to work fine, also when I test the wc-embedder within the docker container.

Still have to check if the gh workflow works without adding a step to create the necessary dist branches.

Copy link
Member

@fgravin fgravin left a comment

Choose a reason for hiding this comment

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

Looking forward to have this, very good and mandatory improvement indeed !

Copy link
Collaborator

@jahow jahow left a comment

Choose a reason for hiding this comment

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

Very well done! 👏 I think the solution looks much better now. Maybe grabbing the version from the package.json should be done in another place as the environments.ts files are not really meant to hold any logic. But that can be done later on IMO.

See if you can address my two comments before merging, that would be great! Thanks again.

.github/workflows/webcomponents.yml Outdated Show resolved Hide resolved
@@ -1,9 +1,14 @@
import packageJson from '../../../../package.json'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be a better practice to import only the version: import { version } from '...'

Otherwise the complete package.json content might end up in the bundle which might be a security issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hum, importing only the version does not seem to work. Following the discussion here I decided to put the logic into the environment.ts to prevent exposing the entire package.json within the code. Checking the produced main.js in production this indeed seems to work, while in dev the package.json content is visible in the debugger.

@tkohr
Copy link
Collaborator Author

tkohr commented Sep 12, 2023

Thanks for your feedback @jahow ! If the last adaptions are ok to you, PR is ready from my side. I adapted the tag version in the doc, as I guess 2.0.0 will be the first version where the versioned wc script will be generated.

Also, if I understand correctly, peaceiris/actions-gh-pages@v3 should take care itself of creating the publish_branch and this does not need to be done in a separate step.

@jahow
Copy link
Collaborator

jahow commented Sep 12, 2023

Yes, that should work :) all good, thanks!

@tkohr tkohr merged commit db3eae9 into main Sep 12, 2023
5 checks passed
@tkohr tkohr deleted the web-components-versioning branch September 12, 2023 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants