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

type: 'module' or format: 'esm' should not require dependencies #1104

Open
whizzzkid opened this issue Nov 2, 2022 · 2 comments
Open

type: 'module' or format: 'esm' should not require dependencies #1104

whizzzkid opened this issue Nov 2, 2022 · 2 comments
Assignees

Comments

@whizzzkid
Copy link
Contributor

Describe the bug
Relates to #1096

dependencies should not be required for transpiled ES Modules. This practice is acceptable, e.g. https://github.com/standard-things/esm/blob/master/package.json

Currently the build breaks: https://github.com/ipfs-shipyard/ipfs-geoip/actions/runs/3381199541/jobs/5614844080

Workaround:

  dependencyCheck: {
    input: [
      'dist/**/*.js',
    ],
    productionInput: [
      'dist/**/*.js',
    ],
  },
@SgtPooki
Copy link
Member

SgtPooki commented Nov 3, 2022

Thanks for jumping on a fix across multiple packages Nishant, great initiative. I have a few concerns though.

Historically, when we build before export, we include the dependencies in the final build.
As I understand it, that is the case for ipfs-shipyard/ipfs-geoip#100, because we're bundling up everything in its entirety and want consumers to be able to use it in the browser. I can get on board with that(ipfs-shipyard/ipfs-geoip#100) move.

Still, I disagree with this issue(#1104), because when consuming a package from Node (such as in webui), the dependencies that are available to source code, say ESM package-A, need to match certain requirements for package-A to function properly, yes?

Without specifying which dependencies are used for building, and which are used for running, consumers of not-ipfs-geoip in NodeJS, who do not bundle their entire dependency tree (and shouldn't) will most likely break if there is a dependency mismatch. Essentially, merging this PR would be obfuscating the problem that peerDeps was intended to help resolve.

Please let me know if I'm missing something!

@whizzzkid
Copy link
Contributor Author

@SgtPooki that is not necessarily true, so this issue #1104 is to just make that check smarter so that it doesn't break builds like it does currently. The way to fix it is not skipping checks in it's entirety but instead to be smart about the checks and looking to the main entry for the package.json being built. e.g. https://github.com/ipfs-shipyard/ipfs-geoip/blob/main/package.json#L20 points to dist/index.min.js which is not importing any of the node modules.

So the idea would be to check the dir for the main entry making the checks more robust.

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

2 participants