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.

Sign up for GitHub

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

ember-source is required for babel-plugin-ember-template-compilation #306

Merged
merged 4 commits into from
Nov 10, 2024
Merged

ember-source is required for babel-plugin-ember-template-compilation #306

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eaa266a
ember-source is required for babel-plugin-ember-template-compilation
NullVoxPopuli Nov 9, 2024
1f4e562
Update index.js
NullVoxPopuli Nov 10, 2024
32a896c
Update package.json
NullVoxPopuli Nov 10, 2024
58d2907
Update package.json
NullVoxPopuli Nov 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions files/__addonLocation__/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@
"@embroider/addon-shim": "^1.8.7",
"decorator-transforms": "^2.0.0"
},
"peerDependencies": {
"ember-source": ">= <%= latestEmberSource %>"
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the rationale to put the version hardcoded into index.js? We have versions everywhere right in the template file (here), so why deviate from this pattern?

I would get it if the versions is determined in a smarter way than being hardcoded, but that's not the case. Seems more confusing to me to maintain tbh.

Copy link
Collaborator Author

@NullVoxPopuli NullVoxPopuli Nov 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I couldn't figure out how to get the version. Maybe i missed something? LatestVersion is async, and the locals object is sync

We have versions everywhere right in the template file (here), s

Where? 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO I would do same what we do in ember-cli addon blueprint - make it second most recent LTS version

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Folks can change it if they want

Copy link
Collaborator

Choose a reason for hiding this comment

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

Where? 😅

Like everywhere in https://github.com/embroider-build/addon-blueprint/blob/main/files/__addonLocation__/package.json? 😅

We have all those versions specifiers hardcoded in that file, why not also ember-source?

IMO I would do same what we do in ember-cli addon blueprint - make it second most recent LTS version

I agree. Also it needs to match what we have in the Readme and whatever ends up in test-app/config/ember-try.js as the minimum Ember version.

Copy link
Collaborator Author

@NullVoxPopuli NullVoxPopuli Nov 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I do want to drop ember older than built in types tho

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will happen in another pr to unblock built in types

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unrelated to this PR tho because we just need a template compiler.

I don't think that's true, as you are introducing a peer dependency on ember-source here, but not on the version we supported until now, which is 4.12, but on >= 5.4. Which means, without violating the peer dependency, you are supposed to require Ember 5.4+ now. But then the addon setup would be very inconsistent, as ember-try and Readme still refer to 4.12.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah I see what you mean

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed 58d2907

},
"devDependencies": {
"@babel/core": "^7.24.4",
<% if (typescript) { %>"@babel/plugin-transform-typescript": "^7.24.4"<% } else { %>"@babel/eslint-parser": "^7.24.1"<% } %>,
Expand Down Expand Up @@ -72,6 +75,7 @@
"@rollup/plugin-babel": "^6.0.4",
"babel-plugin-ember-template-compilation": "^2.2.5",
"concurrently": "^8.2.2",
"ember-source": "^<%= latestEmberSource %>",
"ember-template-lint": "^6.0.0",<% if (packageManager === 'npm') { %>
"ember-eslint-parser": "^0.4.2",
<% } %>"eslint": "^8.56.0",
Expand Down
8 changes: 6 additions & 2 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,7 @@ module.exports = {
pnpm: isPnpm(options),
npm: isNpm(options),
typescript: options.typescript,
latestEmberSource: '5.12.0',
ext: options.typescript ? 'ts' : 'js',
blueprint: 'addon',
blueprintOptions: buildBlueprintOptions({
Expand All @@ -309,7 +310,11 @@ module.exports = {
let files = this._super.files.apply(this, arguments);

if (options.addonOnly) {
files = files.filter((filename) => filename.includes('__addonLocation__') || filesToCopyFromRootToAddonInAddonOnlyMode.includes(filename));
files = files.filter(
(filename) =>
filename.includes('__addonLocation__') ||
filesToCopyFromRootToAddonInAddonOnlyMode.includes(filename),
);
} else {
// filter out the addon-specific npmrc, as it
// is only applicable during --addon-only
Expand Down Expand Up @@ -386,4 +391,3 @@ function isYarn(options) {
function isNpm(options) {
return options.packageManager === 'npm' || options.npm;
}