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

esbuild #229

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

esbuild #229

wants to merge 18 commits into from

Conversation

bencmbrook
Copy link
Member

@bencmbrook bencmbrook commented Jun 16, 2022

Alt to #226 with esbuild only

  • worker is expected to be internally imported so will need to deprecate setWorkerLocation
  • switched mime-type to a lighter browser version (so we don't need to inline the whole mime-db)
  • example, demo, and tests working
  • karma still depends on webpack but could drop it entirely with a separate switch to esbuild
  • may want to add a cjs export as well

Related Issues

  • [none]

Public Changelog

[none]

Security Implications

[none]

@vercel
Copy link

vercel bot commented Jun 16, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
penumbra-example ✅ Ready (Inspect) Visit Preview Jul 16, 2022 at 1:51PM (UTC)


export default WebpackWorker;
class PenumbraWebWorker extends Worker {}
export default PenumbraWebWorker;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

not in use - can delete

@@ -178,7 +178,7 @@ function save(
'stream' in files ? (files as unknown as PenumbraFile) : files[0];
const [
filename,
extension = file.mimetype ? mime.extension(file.mimetype) : '',
extension = file.mimetype ? `.${getExtension(file.mimetype)}` : '',
Copy link
Member Author

@bencmbrook bencmbrook Jun 16, 2022

Choose a reason for hiding this comment

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

lighter mime type lib meant for browsers. also noticed neither library returns with a . prefix which would result in imagepng

@@ -8,6 +8,5 @@ export { default as getTextFromRS } from './getTextFromRS';
export { default as toBuff } from './toBuff';
export { default as blobCache } from './blobCache';
export { default as isViewableText } from './isViewableText';
export { default as intoStream } from './intoStream';
Copy link
Member Author

Choose a reason for hiding this comment

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

Unused + is the only thing depending on web-streams-node, so we can drop it and the npm lib (which itself requires a bunch of polyfills to support)

@eligrey eligrey marked this pull request as ready for review June 22, 2022 07:10
Copy link
Member

@eligrey eligrey left a comment

Choose a reason for hiding this comment

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

esm & cjs tests are now passing

@eligrey
Copy link
Member

eligrey commented Jun 22, 2022

These changes don't deprecate setWorkerLocation just yet. I think we can save that for 6.1

where do I change the code that's calling build:example?
@eligrey
Copy link
Member

eligrey commented Jun 22, 2022

@bencmbrook Where do I change the code that is calling yarn build:example in Vercel? I think that's the only remaining part to address before we can merge this

@bencmbrook
Copy link
Member Author

@eligrey
Copy link
Member

eligrey commented Jul 6, 2022

@bencmbrook thanks; I'll get this unblocked & ready for review by EOW

@bencmbrook
Copy link
Member Author

bencmbrook commented Jul 16, 2022

@eligrey updated that vercel build script to yarn build:dist && cp -a dist/ example/dist/

src/workers.ts Outdated
penumbra: '/worker.penumbra.js',
penumbra: 'worker.penumbra.js',
Copy link
Member Author

Choose a reason for hiding this comment

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

@eligrey is this change ok? I've seen it go back and forth in this PR. With the / the worker 404s in the example

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.

2 participants