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

Support for commonjs missing, Typescript types not packaged by npm #53

Closed
mernxl opened this issue May 23, 2021 · 5 comments
Closed

Support for commonjs missing, Typescript types not packaged by npm #53

mernxl opened this issue May 23, 2021 · 5 comments
Labels
bug Something isn't working

Comments

@mernxl
Copy link

mernxl commented May 23, 2021

Hello, I had these issues when updating to the latest version of this library.

  • Types have not been packaged by npm from the internal typescript index.ts file, i had to revert to using @types/fs-capacitor. Having one less dependency would be nice.
  • Please can you add support for commonjs? Transpiling code to JS using WebPack in commonjs, breaks most of my repositories. Including a .cjs version of your code will really help out a lot.

I believe most node code bases are written in commonjs or use a bundler from Typescript so solving this issue will greatly reduce some headaches.

I can submit a pull request in case my worries are worth looking into. Just wanted to understand your reasons for dropping support for commonjs before making a pull request.

Thanks

@mike-marcacci mike-marcacci added the bug Something isn't working label May 23, 2021
mike-marcacci added a commit that referenced this issue May 23, 2021
This fixed an issue raised in #53, where the published npm package is missing the the TypeScript definition file, despite it being built.
@mike-marcacci
Copy link
Owner

Hi @mernxl, great catch with the missing definition file. This should be fixed by #54.

Regarding publishing of ES modules vs commonjs, the hope for version 7 was to fully transition this (as a "leaf" repository with no production dependencies of its own) to modules, in hopes that it would simplify this transition now that node 10 has reached EOL and all target runtimes support this out of the box.

Can you describe your use-case and setup with webpack a bit more? Given the nature of this project and its heavy reliance on node APIs I was not really expecting it to be used in workflows involving bundlers.

@mernxl
Copy link
Author

mernxl commented May 23, 2021

@mike-marcacci,

Thank you for your prompt reply.

Actually i use WebPack to bundle my typescript API code (actually multiple .ts into a single file) before i deploy to the server.

I try to take advantage of tree-shaking and the minified bundles for production.

Actually as i was exploring different options to load up the es module files, i ended up using webpack-node-externals to manage externals and added fs-capacitor as an allowed package, so its bundled into my final code.

const path = require('path');
const nodeExternals = require('webpack-node-externals');

module.exports = (env) => {
  const RELEASE = env.production;

  return {
    target: 'node',
    externalsPresets: { node: true },
    mode: RELEASE ? 'production' : 'development',
    entry: './src/index.ts',
    devtool: 'inline-source-map',
    module: {
      rules: [
        {
          test: /\.tsx?$/,
          use: 'ts-loader',
          exclude: /node_modules/,
        },
      ],
    },
    resolve: {
      extensions: ['.js', '.ts'],
    },
    externals: [nodeExternals({ allowlist: ['fs-capacitor'] })], // added fs-capacitor so its bundled
    output: {
      filename: 'index.js',
      path: path.join(__dirname, 'build'),
    },
  };
};

Before, i had a small function that returned a list of modules (in node_modules) that were not to be bundled into my code.

So i can say i found a workaround for the issue. I explored multiple options and that is the only on that seems to work.

@mike-marcacci
Copy link
Owner

Ah OK this is good to know. I'm glad you found a workaround; I'm going to go ahead and merge/release that fix for the types, but I'll hold off on adding back commonjs unless this pattern proves to be prevalent.

@mernxl
Copy link
Author

mernxl commented May 24, 2021

Ok, thanks. I guess if someone is using this pattern he may come across this issue. Thanks you.

@mernxl mernxl closed this as completed May 24, 2021
@mike-marcacci
Copy link
Owner

Yes, indeed - thanks for publishing your workaround :) Version 7.0.1 should be out now - do let me know if you have any issues with it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants