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

Document how to use package with require #81

Closed
derberg opened this issue Oct 19, 2022 · 7 comments · Fixed by #87
Closed

Document how to use package with require #81

derberg opened this issue Oct 19, 2022 · 7 comments · Fixed by #87
Labels
bug Something isn't working

Comments

@derberg
Copy link
Member

derberg commented Oct 19, 2022

After migration to TS in readme we focus only on examples with import.

We need examples with require. Especially that before migration const bundle = require('@asyncapi/bundler'); worked well before 0.3 and now it doesn't.

I think we can consider it also a breaking change 🤔

const bundle = require('@asyncapi/bundler');
console.log(bundle)
//{ Document: [Getter], default: [AsyncFunction: bundle] }
//TypeError: bundle is not a function
@derberg derberg added the bug Something isn't working label Oct 19, 2022
aeworxet added a commit to aeworxet/asyncapi-bundler that referenced this issue Oct 20, 2022
@aeworxet
Copy link
Collaborator

With state, the code is currently in

{ Document: [Getter], default: [AsyncFunction: bundle] }

the require import must be written either as

const bundle = require('@asyncapi/bundler').default;

or, if export in index.ts is rewritten as export { Document, bundle }, as

const { bundle } = require('@asyncapi/bundler');

which are both nonsatisfying (because const bundle = require('@asyncapi/bundler'); is needed).

But it seems that the need of having both

export default async function bundle()
export { Document };

in index.ts is to be questioned, as when there is only one export (only bundle) present

export = bundle;

the example TS&JS programs are still able to use document.yml(), for it's exported before, and git reports no differences in generated ./example/asyncapi.yaml.

So it is my assumption that export { Document } can be safely removed from index.ts and replaced with export = bundle;.

@magicmatatjahu
Copy link
Member

magicmatatjahu commented Oct 20, 2022

@aeworxet I wonder if export = bundle works in ESM, is it valid syntax for new module system in JS? I tried to make it in simple app, and I see error:

Export assignment cannot be used when targeting ECMAScript modules. Consider using 'export default' or another module format instead.

However, I opt for exporting bundle function also as named export, so we will have:

export default async function bundle() {...}
export { Document, bundle };

and people in ESM will be able to import it as:

import bundle from '...';
// OR
import { bundle } from '...';

but in CJS as:

const { bundle } = require('...');

@magicmatatjahu
Copy link
Member

I think we can consider it also a breaking change 🤔

The library has not yet reached version 1.0.0 so this is not a breaking change :) Also as far as I know we do not have anywhere done integration of this tool, so there is no problem that someone will not work.

@derberg
Copy link
Member Author

derberg commented Oct 20, 2022

so there is no problem that someone will not work

it is not about will, it basically doesn't work already if someone is using it and updates without checking 😉
we use it in server-api and bump fails

and it has over 2k weekly downloads

anyway, it is breaking change, no matter what version we have, just for the record 😄
but I do know it did not reach 1.0.0 yet so it is kinda ok, this is why I reported it as a docs bug issue.

I'm ok if the world will look like:

const { bundle } = require('...');

more important is to fix and document it

aeworxet added a commit to aeworxet/asyncapi-bundler that referenced this issue Oct 21, 2022
@aeworxet
Copy link
Collaborator

aeworxet commented Oct 21, 2022

The change

export = bundle;

will be applied to source file index.TS (it is a TS-only syntax), and it will be transpiled to index.JS as

module.exports = bundle;

during build stage.
After that, this export will be available for usage to any JS (both CJS and a backwards-compatible ESM) app.
Which at the same time means that Node.js import will work as it did before, prior to v0.3.0:

const bundle = require('@asyncapi/bundler');

so it will also be a bugfix for the breaking change.

I have temporarily removed /lib from .gitignore and committed transpiled directory lib, so the result of future building of NPM package can be checked.

I also included two files

bundle-cjs.js
bundle-esm.js

to clearly illustrate to users the difference (both these files can also be included in future documentation).

When continuing experimentation, specifying

"module": "ES6",

in ./tsconfig.json had broken imports in library itself

which would be a new breaking change, if used.

and TSConfig / Module documentation states
You very likely want "CommonJS" for node projects.
https://www.typescriptlang.org/tsconfig#module

I'm assuming the package will target exactly Node.js projects, but might there be any other uses predicted?

@aeworxet
Copy link
Collaborator

New version of the bugfix for the breaking change.
It uses familiar

module.exports = bundle;

syntax, is easily transpiled with Babel

(export = bundle could not be transpiled by Babel because it is a TypeScript-only syntax, which has no analogue in JavaScript, thus Babel was simply throwing an exception during npm run generate:assets)

  1. This plugin does not support export = and import =, because those cannot be compiled to ES.next. These are a TypeScript only form of import/export.
    https://babeljs.io/docs/en/babel-plugin-transform-typescript

and can be used by both example/bundle-cjs.js and example/bundle.ts.

To achieve this, generation of TS declaration files during build stage was removed, as they do not seem to be needed in a JS NPM package anyway, but were confusing example/bundle.ts.

aeworxet added a commit to aeworxet/asyncapi-bundler that referenced this issue Oct 24, 2022
aeworxet added a commit to aeworxet/asyncapi-bundler that referenced this issue Oct 24, 2022
aeworxet added a commit to aeworxet/asyncapi-bundler that referenced this issue Oct 25, 2022
aeworxet added a commit to aeworxet/asyncapi-bundler that referenced this issue Oct 25, 2022
aeworxet added a commit to aeworxet/asyncapi-bundler that referenced this issue Oct 26, 2022
…ncapi#81)

Co-authored-by: Maciej Urbańczyk <urbanczyk.maciej.95@gmail.com>
Co-authored-by: Lukasz Gornicki <lpgornicki@gmail.com>
@aeworxet
Copy link
Collaborator

It turned out that

export default async function bundle()

and

module.exports = bundle;

co-habitate in the code just fine.

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

Successfully merging a pull request may close this issue.

3 participants