Skip to content

Remove entryPoint if not required#334

Merged
greggman merged 5 commits intowebgpu:mainfrom
beaufortfrancois:entryPoint
Mar 8, 2024
Merged

Remove entryPoint if not required#334
greggman merged 5 commits intowebgpu:mainfrom
beaufortfrancois:entryPoint

Conversation

@beaufortfrancois
Copy link
Copy Markdown
Collaborator

See gpuweb/gpuweb#4387

⚠️ Do not merge this PR until Chrome 121 reaches stable channel.

@Kangz
Copy link
Copy Markdown
Contributor

Kangz commented Nov 27, 2023

FYI @jimblandy @mwyrzykowski this will simplify samples a bit but will require entryPoint name defaulting.

@kainino0x kainino0x marked this pull request as draft November 28, 2023 02:41
@kainino0x
Copy link
Copy Markdown
Collaborator

Converting to draft to make the PR harder to accidentally merge.

@beaufortfrancois beaufortfrancois marked this pull request as ready for review January 25, 2024 08:39
@beaufortfrancois
Copy link
Copy Markdown
Collaborator Author

We can now merge it as Chrome 121 is stable.

austinEng
austinEng previously approved these changes Jan 25, 2024
@greggman
Copy link
Copy Markdown
Collaborator

ps: Firefox still requires entryPoint as of nightly 124.0a1 (2024-01-25) (64-bit)

@kdashg @jimblandy

Copy link
Copy Markdown

@kdashg kdashg left a comment

Choose a reason for hiding this comment

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

Just a sec!

@jimblandy
Copy link
Copy Markdown

So, we agree this is a sweet readability hack, and would like to see it deployed in the examples - but since it will break Firefox, we would like to wait to merge this PR until gfx-rs/wgpu#5145 is closed.

@austinEng austinEng self-requested a review January 26, 2024 17:56
@austinEng austinEng dismissed their stale review January 26, 2024 17:57

waiting for firefox to update

@jimblandy
Copy link
Copy Markdown

Note that we need both gfx-rs/wgpu#5145 (the wgpu-side work) and Bugzilla Bug 1877488 (the Firefox bindings work) fixed before Firefox will be able to handle omitted entry point names. @ErichDonGubler is actively working on these.

@beaufortfrancois
Copy link
Copy Markdown
Collaborator Author

Firefox Nightly now handles omitted entry point names (I've checked locally).
Since Safari Tech Preview and Chrome also handle them properly, I believe we can now merge this PR.

Note that I've just rebased it due to merging conflicts.

@ErichDonGubler
Copy link
Copy Markdown

As the person who worked on this support in Firefox, I'm excited to see this land. 😁

@beaufortfrancois
Copy link
Copy Markdown
Collaborator Author

I've had to update the PR again. Please review and merge ;)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Are the changes to package-lock.json intentional?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes. They are due to me running npm i because of iframe changes.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

But it doesn't look like any of the code changes should affect the packages? Is it that main has a stale package-lock.json? If so, can we do that as a separate PR?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

You're right. I'll revert package-lock.json changes then to not pollute this PR.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done

@beaufortfrancois
Copy link
Copy Markdown
Collaborator Author

@kainino0x Can you merge?

@greggman
Copy link
Copy Markdown
Collaborator

greggman commented Mar 8, 2024

I'll merge

@greggman greggman merged commit 92bb1a3 into webgpu:main Mar 8, 2024
@beaufortfrancois beaufortfrancois deleted the entryPoint branch March 8, 2024 08:13
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.

10 participants