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

Fix source map option not being passed to plugin #1787

Merged
merged 2 commits into from
Feb 5, 2024

Conversation

vstefanovic97
Copy link
Contributor

@vstefanovic97 vstefanovic97 commented Feb 2, 2024

Sorry about that, checked and now it works 100%
Missed it in #1786 thought plugin was just getting re-exported, my mistake

@SergeAstapov SergeAstapov added the bug Something isn't working label Feb 2, 2024
@@ -57,8 +57,8 @@ export class Addon {
return hbs();
}

gjs() {
return gjs();
gjs(options: { inline_source_map: true }) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@vstefanovic97 shouldn’t we mark this as optional?

Suggested change
gjs(options: { inline_source_map: true }) {
gjs(options?: { inline_source_map: true }) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♂️ , I'll fix it right away

gjs() {
return gjs();
gjs(options?: { inline_source_map: true }) {
return gjs(options);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any downside to defaulting this to true?

We already have rollup's sourcemaps enabled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm well probably not, I think I can default it to true in this case if you want @NullVoxPopuli

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the default to true @NullVoxPopuli

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry for all of these tiny mistakes and having to do release patches.... my brain not working well these past 2 days 🤦‍♂️ , would have made another mistake if @SergeAstapov didn't catch it

Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli Feb 2, 2024

Choose a reason for hiding this comment

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

I mean, in your defense, we don't have sufficient testing for addon-dev in this repo

@vstefanovic97
Copy link
Contributor Author

@NullVoxPopuli can we merge and release patch ?

@NullVoxPopuli NullVoxPopuli merged commit d36a36a into embroider-build:stable Feb 5, 2024
201 checks passed
@github-actions github-actions bot mentioned this pull request Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants