-
Notifications
You must be signed in to change notification settings - Fork 32
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
Conversation
Affected libs:
|
There was a problem hiding this 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.
…well note: might need an additional action to create the release branches used in publish_branch if they do not exist yet
d64d4f6
to
fb81e97
Compare
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 Still have to check if the gh workflow works without adding a step to create the necessary dist branches. |
There was a problem hiding this 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 !
There was a problem hiding this 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.
@@ -1,9 +1,14 @@ | |||
import packageJson from '../../../../package.json' |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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 Also, if I understand correctly, |
Yes, that should work :) all good, thanks! |
The goal of this PR is to introduce versioning to gn-ui web components. It should publish the
gn-wc.js
script to awc-dist-main
branch upon merges onmain
and towc-dist-[tagname]
branches when creating version tags complying to'v*.*.*'
.Potential to-dos:
publish_branch
, if it does not exist yet.