-
Notifications
You must be signed in to change notification settings - Fork 30
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
convert addon to v2 #908
convert addon to v2 #908
Conversation
345257e
to
a593258
Compare
@@ -1,9 +1,6 @@ | |||
import { module, test } from 'qunit'; | |||
import sinon from 'sinon'; | |||
import { |
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 safe to me as those are not public API?
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.
@SergeAstapov from what I can see they are used as test helpers for the test app, so probably is fine?
addon/rollup.config.mjs
Outdated
@@ -0,0 +1,40 @@ | |||
import ts from 'rollup-plugin-ts'; |
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.
@vstefanovic97 could you please latest addon-blueprint where we migrated away from rollup-plugin-ts
?
addon/package.json
Outdated
@@ -66,13 +75,25 @@ | |||
"engines": { | |||
"node": "14.* || 16.* || >= 18" | |||
}, | |||
"typesVersions": { | |||
">=5.0.0": { |
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.
why it's not
">=5.0.0": { | |
"*": { |
addon/package.json
Outdated
@@ -66,13 +75,25 @@ | |||
"engines": { |
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.
now this field can be removed as there is nothing about Node.js for v2 addons
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 not need to install node in ci.yml then?
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.
we indeed Node.js to precompile addon and run tests in test-app, it's just now we could bump node version in ci.yml without need to do a major version bump if that's what you meant.
addon/package.json
Outdated
".": "./dist/index.js", | ||
"./*": "./dist/*.js", | ||
"./test-support": "./dist/test-support/index.js", | ||
"./addon-main.cjs": "./addon-main.cjs" |
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.
isn't this
"./addon-main.cjs": "./addon-main.cjs" | |
"./addon-main.js": "./addon-main.cjs" |
.github/workflows/ci.yml
Outdated
- name: Precompile Typescript | ||
run: yarn workspace ember-sinon-qunit run prepack | ||
- name: Build v2 Addon | ||
run: yarn workspace ember-sinon-qunit run build |
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.
maybe we can rely on prepare
script like it's done in blueprint https://github.com/embroider-build/addon-blueprint/blob/main/src/root-package-json.js#L52 ?
Do we need to update our dependabot config to ensure it can create PRs bumping versions in |
No need to, dependabot handles monorepos with no extra setup |
e460235
to
2e87764
Compare
No description provided.