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

Webpack entry cannot be found after importing filer #790

Closed
thomas-jakemeyn opened this issue Nov 12, 2021 · 15 comments
Closed

Webpack entry cannot be found after importing filer #790

thomas-jakemeyn opened this issue Nov 12, 2021 · 15 comments

Comments

@thomas-jakemeyn
Copy link

Context

I am trying to add FilerWebpackPlugin to an existing Webpack configuration.

Problem

As soon as I require('filer'), Webpack starts failing because it cannot find the entry file anymore.

Analysis

This problem seems to be caused by a side effect in src/path.js:

/**
 * Patch process to add process.cwd(), always giving the root dir.
 * NOTE: this line needs to happen *before* we require in `path`.
 */
process.cwd = () => '/';

Workaround

The only workaround that I found is to save process.cwd before importing filer, and to restore it afterwards.

const cwd = process.cwd;
const filer = require('filer');
process.cwd = cwd;

With this fix, Webpack is able to find the entry file again.

@humphd
Copy link
Contributor

humphd commented Nov 13, 2021

cc @bcheidemann, who might have thoughts.

@bcheidemann
Copy link
Contributor

bcheidemann commented Nov 13, 2021

Thanks for raising this issue @thomas-jakemeyn.

The problem seems to be that we import the whole of filer when we only need the FilerWebpackPlugin class. In hindsight, this is obviously not the best idea! I will put up a PR to deprecate accessing the plugin through the "normal" filer import and add a specific import path for the plugin. @humphd what do you think about:

const { FilerWebackPlugin } = require('filer/webpack');

@thomas-jakemeyn Until the fix is released, you should be able to import the class directly:

// Direct import
const FilerWebpackPlugin = require('filer/src/webpack-plugin');

Let me know if this works for you! :)

@thomas-jakemeyn
Copy link
Author

Hello @bcheidemann,

I confirm that using a direct import as you suggested works. Thank you for that!
However, don't you think that the real problem is the side-effect on process.cwd?

@bcheidemann
Copy link
Contributor

Hello @bcheidemann,

I confirm that using a direct import as you suggested works. Thank you for that! However, don't you think that the real problem is the side-effect on process.cwd?

@thomas-jakemeyn The process.cwd side effect is, as I understand it, required for filer to work properly in the browser. Maybe @humphd can clarify the exact purpose? Regardless, the Webpack config only needs to load the FilerWebpackPlugin class which does not import any part of the main filer library (nor should it). As such, if imported correctly, there should be no side-effects.

Are you having trouble with the process.cwd side effect other than in the context of the Webpack config (i.e. at runtime)?

@thomas-jakemeyn
Copy link
Author

The problem that I was personally facing is solved with your suggestion.

@humphd
Copy link
Contributor

humphd commented Nov 15, 2021

re: the process.cwd override, that can probably be removed. It's used very rarely, and likely could be dealt with through documentation alone. Unlike fs running in the OS, we have no concept of a process on which the current working directory can be managed. We could probably move this to the shell instead. I'd take a PR that did this, but am unlikely to have time to do it any time soon.

@bcheidemann
Copy link
Contributor

Thanks for the clarification @humphd.

@thomas-jakemeyn do you anticipate other issues that wouldn't be solved by PR #793? Happy to rethink our approach if there's reason to do so :)

@thomas-jakemeyn
Copy link
Author

Since filer is intended to be used in a browser, I guess that it should be fine indeed. Thank you for the clarification and for the quick reaction!

@bcheidemann
Copy link
Contributor

bcheidemann commented Nov 15, 2021

No problem!

@humphd shall we proceed with PR #793 so we can close this issue?

EDIT: I have seen the other issue about the shims not being present in the version distributed through npm - don't have time to address that until this evening or tomorrow but we should probably ship any fix for that with the fix for this issue imo

humphd pushed a commit that referenced this issue Nov 15, 2021
* fix: make wepback plugin available from filer/webpack

* deprecate accessing FilerWebpackPlugin through index.js

* docs: update documentation to reflect changes to FilerWebpackPlugin

* docs: fix typo
@humphd
Copy link
Contributor

humphd commented Nov 15, 2021

Landed #793, I'll ship v1.4.0 in a min.

@humphd
Copy link
Contributor

humphd commented Nov 15, 2021

@humphd humphd closed this as completed Nov 15, 2021
@bcheidemann
Copy link
Contributor

@bcheidemann
Copy link
Contributor

@humphd I think this issue needs to be reopened. I just checked the latest NPM release (v1.4.0) and the new import is not working. This is because I added a root folder called "webpack" which wasn't released to NPM. Could you advise on what I need to change so that this is included in the release to NPM? This is also the case for the "shims" directory which is the reason for issue #791

@humphd
Copy link
Contributor

humphd commented Nov 15, 2021

Should be fixed by v1.4.1

@humphd humphd closed this as completed Nov 15, 2021
@bcheidemann
Copy link
Contributor

Should be fixed by v1.4.1

Confirmed, the correct files have been pushed to NPM

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

No branches or pull requests

3 participants