-
Notifications
You must be signed in to change notification settings - Fork 40
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/logo building #89
Conversation
* feat: flag for committing * feat: adding cdn/2.2.0-alpha.1 * chore: added commit variable and adressed review comments * chore: addressing PR comments * chore: dropped -a flag * chore: updated commit to a chore * feat: updated workflow to run buildscript
76e528a
to
b2fe5cd
Compare
force push to fix commit-lint issue |
Co-authored-by: Greg Jopa <534034+gregjopa@users.noreply.github.com>
b2fe5cd
to
7961085
Compare
Codecov ReportBase: 67.64% // Head: 70.31% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #89 +/- ##
==========================================
+ Coverage 67.64% 70.31% +2.67%
==========================================
Files 41 41
Lines 204 283 +79
Branches 34 35 +1
==========================================
+ Hits 138 199 +61
- Misses 64 81 +17
- Partials 2 3 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
a53bfc6
to
d80181d
Compare
return JSON.parse(fs.readFileSync("./package.json", "utf-8")); | ||
} | ||
|
||
export function getNodeOps(): NodeOps { |
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.
Do we use this getNodeOps()
function anywhere? If not, can we delete it?
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.
Ah right. Should we use it to update the namespace in the CDN URL, like we do with package version? I like the idea of making it dynamic but it's another point of failure as well.
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 the .nodeops file can remain static similar to what we do in paypal/sdk-release.
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 thought it might be confusing if somebody ever tried to change the namespace and the urls didn't update. I don't see why we would ever need to change the namespace, though
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.
oh I see your point. We can keep it then to validate the namespace name since its directly tied to the cdn url.
Co-authored-by: Greg Jopa <534034+gregjopa@users.noreply.github.com>
Co-authored-by: Greg Jopa <534034+gregjopa@users.noreply.github.com>
@@ -21,7 +21,8 @@ | |||
"clean": "rimraf dist coverage", | |||
"reinstall": "rimraf flow-typed && rimraf node_modules && npm install && flow-typed install", | |||
"debug": "cross-env NODE_ENV=debug", | |||
"prepare": "husky install" | |||
"prepare": "husky install", | |||
"build-logos": "babel-node --config-file './scripts/babel.config.json' --ignore ./fake --plugins=transform-es2015-modules-commonjs ./scripts/buildLogos.js" |
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.
What does this --ignore ./fake
part do? Can we remove it?
"build-logos": "babel-node --config-file './scripts/babel.config.json' --ignore ./fake --plugins=transform-es2015-modules-commonjs ./scripts/buildLogos.js" | |
"build-logos": "babel-node --config-file './scripts/babel.config.json' --plugins=transform-es2015-modules-commonjs ./scripts/buildLogos.js" |
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.
Added an ignore flag to fix an issue with babel-node:
/Users/dustijones/code/public/paypal-sdk-logos/node_modules/@krakenjs/belter/src/index.js:3
export * from "./device";
^^^^^^
I'll see if I can find a long-term solution
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.
Looks like it may be a bug that occurs with babel-node
when you use the --config-file
flag.
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.
Good find. We can keep this. I just wanted to understand why it is needed.
Co-authored-by: Greg Jopa <534034+gregjopa@users.noreply.github.com>
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.
Amazing work! 💯
await $`git commit -m "chore: generate CDN packages"`; | ||
await $`git push`; | ||
} | ||
} |
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.
👍
DTPPSDK-997
build-logos
script has been added, which creates a versioned CDN folder populated with APM SVGs and (optionally) commits to github. This script has been added as a step in thepublish
github action.checkout-components
(loadFromCDN
). As an example: