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 node command error caused by type: module in package.json #740

Merged
merged 1 commit into from
Jun 28, 2023
Merged

Fix node command error caused by type: module in package.json #740

merged 1 commit into from
Jun 28, 2023

Conversation

osbre
Copy link
Contributor

@osbre osbre commented Jun 2, 2023

Since the official Laravel starters (Jetstream and Breeze) now have this line in package.json,

laravel/jetstream#1315

"type": "module",

This package no longer works in new Laravel projects because the node command execution results in an error.

The solution would be rename the file to cjs which will indicate that this file runs CommonJS code.

@osbre osbre changed the title Fix for type: module in package.json Fix package for type: module in package.json environments Jun 2, 2023
@osbre osbre changed the title Fix package for type: module in package.json environments Fix package for "type: module in package.json" environments Jun 2, 2023
@osbre osbre changed the title Fix package for "type: module in package.json" environments Fix error for "type: module in package.json" environments Jun 2, 2023
@osbre osbre changed the title Fix error for "type: module in package.json" environments Fix node command error caused by type: module in package.json Jun 2, 2023
@timvandijck timvandijck self-requested a review June 8, 2023 18:39
@timvandijck
Copy link
Member

@osbre seems like a good fix, thank you for the contribution. Would you pulling the latest version from main into your branch?

@osbre
Copy link
Contributor Author

osbre commented Jun 9, 2023

Thanks @timvandijck, I've updated the branch

@osbre
Copy link
Contributor Author

osbre commented Jun 23, 2023

@timvandijck Here is a similar change made to the official Laravel package: laravel/octane#696

@ingLomeland
Copy link

@timvandijck any updates on when this can be merged?

@ingLomeland
Copy link

maybe even @freekmurze could have a look? its currently broken for Laravel ^10.8 without this change

@freekmurze
Copy link
Member

Will this break for people on older Laravel versions?

@ingLomeland
Copy link

Will this break for people on older Laravel versions?

Unknown.

@osbre
Copy link
Contributor Author

osbre commented Jun 27, 2023

Will this break for people on older Laravel versions?

it shouldn't unless they have a reference to vendor/spatie/browsershot/bin/browser.js in any way in the codebase

btw, the tests are failing for unrelated reason and need to be re-run.

@freekmurze freekmurze merged commit 2d46fd9 into spatie:main Jun 28, 2023
@freekmurze
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants