-
-
Notifications
You must be signed in to change notification settings - Fork 134
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
Upgrade to fs-capacitor v4 #166
Conversation
I've done a rework of fs-capacitor to support node 13. This involved using the `readable-stream` package instead of node's stream library in order to simply continued compatibility with node v8.x, which we can drop at the end of 2019. I spent the better part of a day trying to figure out exporting interoperable es6 and commonjs modules now that I have a commonjs dependency, and I finally just gave up. Without any useful guidance or pathway put forward by node for interoperability, the only choice here is to chose a single module system, which will be commonjs for the immediate future. I'm very unhappy with this situation and fear this is going to severly fragment the ecosystem, but don't have the time to get involved any further.
OK, I've worked around all the quirks here and have everything passing on all target versions in docker. It appears that there is a flaw in a newly released dependency of eslint which is causing these failures on CI but is unrelated. |
I saw you just published a new major version of Additionally, this commit in eslint prevents it from running on node EDIT: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for getting on top of Node.js v13! I'm not really across all the stream related changes, so I'll defer to your knowledge there and trust the tests.
Could you please add a changelog.md
entry? Particularly important is to document the breaking changes (if there are any that are consumer facing).
After merging this PR, I might strip out all the ESM stuff and maybe even get rid of the build step, and just ship vanilla CJS. Alternatively, we could keep the build step, but output .js
files to a seperate ESM directory and create a package module
entry just for bundlers. Considering this is a lightweight, server-only package, I'm not sure that level of complexity is worth it. It would retain tree shaking for people that bundle their server code and only want to only include the relevant middleware.
OK, I added the change log. It appears that the tap/nyc combo are not respecting the Technically the engine bump from node 8.5 to node 8.10 is a breaking change. If you are instead interested in cutting a new major that adds support for node 13 but drops support for node 8 (essentially a month and a half before its LTS end of life), I've already published Because there are not big feature changes or fixes here, we could simply continue to "support" graphql-upload version 8.x through the end of the year (and expect other tools like Apollo to continue using it for that timeline), while publishing version 9 with support for node 10 through 13. If you're interested in this strategy, I'll update this PR to use |
I also fixed up the changelog entry. # Conflicts: # .github/workflows/ci.yml # package.json
I removed tests that checked for cleanup of internal files used by fs-capacitorr. These tests required hooks into internal details of the library and are really outside the scope of concerns for this project.
The event handlers were unintentionally returning.
Awesome 🙌 |
I've done a rework of fs-capacitor to support node 13. This involved using the
readable-stream
package instead of node's stream library in order to simply continued compatibility with node v8.x, which we can drop at the end of 2019.I spent the better part of a day trying to figure out exporting interoperable es6 and commonjs modules now that I have a commonjs dependency, and I finally just gave up. Without any useful guidance or pathway put forward by node for interoperability, the only choice here is to chose a single module system, which will be commonjs for the immediate future.
@jaydenseric I'll bet you have some thoughts on this matter, which I'd love to hear.
I've also opened an issue in
fs-capacitor
to track how to maintain support for es modules.Regardless, this is updated to work with the newest
fs-capacitor
and adds compatibility with node v13.