filerjs / filer

Node-like file system for browsers
BSD 2-Clause "Simplified" License
613 stars 153 forks source link

Migration from Parcel-Bundler 1.12.5 to Parcel 2 #796

Open joshuali7536 opened 2 years ago

joshuali7536 commented 2 years ago

Fixes #794

As discussed in the issue above I made changes to the packages to use Parcel 2. This was done by following the migration instructions on Parcel's website. This included the following changes:

joshuali7536 commented 2 years ago

Yeah I was still working on making all the changes sorry. Just wanted to make the draft pull request and make changes as I go. I've made the changes to --out-dir already. Just working on making sure everything else runs when I run npm test.

joshuali7536 commented 2 years ago

@humphd for the migration, Parcel 2 doesn't use the --out-file CLI option anymore as described here. But I'm not really 100% sure if I'm doing it correctly at lines 18-19 and updating the scripts at lines 33-34. After testing it, it works but I just want to double check with you.

Also, I ran into multiple errors when trying to build it while running the npm test like the following below:

$ npm test
Building...
× Build failed.

@parcel/core: Failed to resolve 'path' from './src/path.js'

  C:\Users\Joshua\Documents\GitHub\filer\src\path.js:12:26
    11 |  */
  > 12 | const nodePath = require('path');
  >    |                          ^^^^^^
    13 | const filerPath = Object.assign({}, nodePath);
    14 |

@parcel/resolver-default: External dependency "path" is not declared in package.json.

  C:\Users\Joshua\Documents\GitHub\filer\package.json:49:3
    48 |   },
  > 49 |   "dependencies": {
  >    |   ^^^^^^^^^^^^^^
    50 |     "buffer": "^6.0.3",
    51 |     "chai": "^4.3.4",

  ℹ Add "path" as a dependency.

So I had to add a bunch of packages to the dependencies to package.json and even just some paths to get the test to work properly. This also includes some stuff that has already been declared in other parts of the package.json file like chai or chai-datetime, and other things like node modules that normally didn't need to be declared like buffer, path, and process. I'm not sure why it is doing this, I'm not sure if this is because Parcel can only see inside the dependencies or if I did something wrong.

humphd commented 2 years ago

path is a node built-in, and according to https://parceljs.org/features/dependency-resolution/#builtin-modules should get resolved. I'm not sure what the issue is there, it's worth investigating more (i.e., this should work imho).

joshuali7536 commented 2 years ago

Yeah after reading the documentation and messing around with the package.json, I still couldn't figure out why it needed for these built-in node modules to be added to the dependencies. And I couldn't find anyone else running into this issue in a google search. I agree that node modules should simply work which is weird that it doesn't work without having them in the dependencies of package.json. As the documentation specifies, the bare specifiers should automatically search the node_modules folder for the path module when `require('path') is called but some reason it doesn't seem to see it?

The tests work fine with the current changes to package.json, where we have added the necessary packages to the dependencies. Are you ok with having the additional packages in the package.json?

humphd commented 2 years ago

No, we can't use path as a dep, https://www.npmjs.com/package/path is 6 years out of date.

I would file a bug in Parcel's repo and ask them why this doesn't work. Probably we have to do something that isn't obvious.

humphd commented 2 years ago

Now that they merged that fix, we should try it by setting the package.json version to parcel-bundler/parcel#4fbc352b40 (i.e., until they do a new release) see https://docs.npmjs.com/cli/v8/configuring-npm/package-json#github-urls

Notplayingallday383 commented 1 month ago

why not use ES Build as its almost a drop in replacement in this case. it worked fine in my testing.