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

Migration from Parcel-Bundler 1.12.5 to Parcel 2 #796

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

joshuali7536
Copy link

@joshuali7536 joshuali7536 commented Nov 17, 2021

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:

  • modifying the package dependency name and version in package.json
  • modifying the .cache to .parcel-cache in .gitignore

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

The old setup relies on:

error: unknown option '--out-dir'
Error: Process completed with exit code 1.

We need a solution with Parcel 2

package.json Outdated
@@ -65,14 +65,13 @@
"meow": "^10.0.1",
"mocha": "^9.1.3",
"nyc": "^15.1.0",
"parcel-bundler": "^1.12.5",
"parcel": "^2.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Parcel is at 2.0.1 right now, so we might as well use that, https://www.npmjs.com/package/parcel

@joshuali7536
Copy link
Author

joshuali7536 commented Nov 17, 2021

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
Copy link
Author

@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
Copy link
Contributor

humphd commented Nov 17, 2021

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
Copy link
Author

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
Copy link
Contributor

humphd commented Nov 17, 2021

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.

…endencies when they are available already in node_modules internally
@humphd
Copy link
Contributor

humphd commented Nov 23, 2021

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

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.

Critical dependency: the request of a dependency is an expression
2 participants