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: Deno and cloudflare workers deploy #1265

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

riderx
Copy link

@riderx riderx commented Feb 12, 2024

As I reported here: #1261
There is some issue in Deno and also in Cloudflare workers to deploy.

I used a hack with ESM to deploy, but found out the only problem was the Babel config.

The good part is it's easy to fix, since it's already stated in package.json that the min version to use the package is Node.js 16.

The node: was introduced in Node.js 16.

Since then, it also got back ported to Node.js 14 and Node.js 12, but I followed what was set in package.json and aligned the test to run only from 16.

I also updated the lint step to run on the latest LTS, that is optional I can remove it if required.

Link: who talks about supported version: https://2ality.com/2021/12/node-protocol-imports.html

Node.js doc: https://nodejs.org/api/esm.html#node-imports

@riderx
Copy link
Author

riderx commented Feb 12, 2024

A Pr will be done also in submodules xml2js and xml to apply the same fix

@riderx
Copy link
Author

riderx commented Feb 12, 2024

I found out you where using fast-xml-parser in your test but not in the lib.
This lib is compatible with all env and requires a minimum number of changes.
I made the change and it also reduced the bundle size:
CleanShot 2024-02-12 at 20 27 13@2x
The test passed:
CleanShot 2024-02-12 at 21 57 22@2x
I found only one change during my test:
<?xml version="1.0" encoding="UTF-8" standalone="yes"?> header is never present with the new lib.
But it seems it passed your test.
It should be double checked to be sure it's working without the header.
If it's required, i can update the code

@riderx riderx mentioned this pull request Feb 12, 2024
@riderx
Copy link
Author

riderx commented Feb 13, 2024

I also had to replace stream-json for the same reason, alternative was recommended here: uhop/stream-json#91

@riderx
Copy link
Author

riderx commented Feb 13, 2024

Last require change: mime-types to mime package
For that one, I honestly didn't want to change it since the change to the original repo was minimal: riderx/mime-types@fdb301e
PR here: jshttp/mime-types#120
But the package creator is against that change, since express is still supporting node 0.10:
jshttp/mime-types#115
I made the change he suggested, and put mime package.

That led me to make the big change you see in the last commit.
I had to switch to type module and then the build script was totally broken.
The easy way was, for me, to use a more modern builder.
I hope this will not become too big of a change.

@pellicceama
Copy link

It would be awesome to prioritize this to make it work on Cloudflare workers.

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