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

No support for CommonJS #696

Closed
indexofrefraction opened this issue Nov 20, 2022 · 9 comments · Fixed by #700 or #713
Closed

No support for CommonJS #696

indexofrefraction opened this issue Nov 20, 2022 · 9 comments · Fixed by #700 or #713
Assignees
Labels
bug Priority: 1

Comments

@indexofrefraction
Copy link

indexofrefraction commented Nov 20, 2022

hi,

i'm using nw-builder inside a macos node js shell command "nwbuild".
since 4.0.0 var NwBuilder = require('nw-builder'); is not supported anymore (Error ERR_REQUIRE_ESM)
but if i add type: "module" to the package.json and change to: import { nwbuild } from 'nw-builder';
i get Error ERR_UNKNOWN_FILE_EXTENSION because im running "nwbuild" without extension

is there a way around changing the commands name to nwbuild.js ?

if i do and replace:

#!/usr/bin/env node
var NwBuilder = require('nw-builder');
var settings = {
	files: '_build/\*\*/\*\*',
	platforms: ['osx64', 'win32'], flavor: 'normal', version: 'latest', 
	buildDir: '_nwbuild',  cacheDir: '_nwcache', zip: true
}
var nw = new nwbuild(settings); 
nw.build().then(function () {
	console.log("Promise Resolved");
}).catch(function () {
	console.log("Promise Rejected");
});

by

#!/usr/bin/env node
import { nwbuild } from 'nw-builder';
var settings = {
	srcDir: "_build",
	platform: 'osx', arch: 'x64', flavor: 'normal', version: 'latest',
	outDir: '_nwbuild', cacheDir: '_nwcache', zip: true
};
nwbuild(settings).then(function () {
	console.log("Promise Resolved");
}).catch(function () {
	console.log("Promise Rejected");
});

i get this :
% nwbuild_test.js
[ INFO ] Manifest file already exists locally under _nwcache
■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■ 100% | ETA: 0s | 22833/22833
Promise Rejected

a small porting guide from 3.x to 4.x would be nice
what is the problem with the code?
do the platforms need to be built separately now?
what about arm64 / universal on osx?

@sysrage sysrage changed the title documentation on changes and how to use 4.0.0 ? No support for CommonJS Nov 20, 2022
@sysrage sysrage added the feature Priority: 2 label Nov 20, 2022
@sysrage
Copy link
Contributor

sysrage commented Nov 20, 2022

I just encountered this myself and was annoyed that CJS wasn't supported (especially since NW.js doesn't support ESM for the main script). I've updated the title and I'm hijacking this Issue to request that CJS be supported. I don't have specifics handy, but it's possible for a package to support both ESM and CJS out-of-the-box.

That said @indexofrefraction, you can simply name your build script with the .mjs extension and it'll be treated as an ESM module by Node.js. That way, you don't need to include "type": "module" in package.json, which wouldn't be accurate for your NW.js script.

Additionally, I do have this working, so you should log your error to see what the problem is:

nwbuild(settings).then(function () {
	console.log("Promise Resolved");
}).catch(function (error) {
	console.log("Promise Rejected", error);
});

It's possible you're running into Issue #692, but I don't think that was rejecting (just never resolving).

For a working (when Issue #692 is fixed) example, see my nw-build branch here: https://github.com/sysrage/nw-react/blob/nw-build/README.md

I'll be pushing these changes to main and transferring nw-react to nwutils, once issue #692 is resolved.

@sysrage
Copy link
Contributor

sysrage commented Nov 20, 2022

As an aside, I also ran into the issue of only one platform being supported. I suggest opening a separate issue to track that request, if it's important. As you can see in the dist.mjs script in nw-react, I just work around this by calling nwbuild() multiple times. This ultimately works out better, so I can change file/directory names and application content depending on the platform.

@indexofrefraction
Copy link
Author

hi,
tx for tip about the mjs extension. (but I wish i wouldn't need that / run it without extension)
LOL, and the error i get is just : 1

@sysrage
Copy link
Contributor

sysrage commented Nov 20, 2022

Ya, the error only being "1" makes sense (but should also be fixed):

@tharatau, you should probably be calling reject(error) instead of reject(1) in both these spots. The values you're returning with reject() and resolve() aren't very helpful.

@indexofrefraction
Copy link
Author

maybe to add... if the cache is cleared, the above script downloads manifest.json and nw.zip
and then exits with "[ INFO ] Manifest file already exists locally under _nwcache" and error 1

@ayushmanchhabra ayushmanchhabra added bug Priority: 1 and removed feature Priority: 2 labels Nov 20, 2022
@ayushmanchhabra ayushmanchhabra self-assigned this Nov 20, 2022
@sysrage
Copy link
Contributor

sysrage commented Nov 21, 2022

Your fix for this isn't how ESM/async works. Example:

const { nwbuild } = require('nw-builder');

console.log('nwbuild', nwbuild);

You will see nwbuild undefined in the console, because the exported variable is undefined until the async IIFE in nwbuild.cjs resolves. Obviously, attempting to call nwbuild() also fails.

There are ways to support both CJS and ESM, but this isn't right. I believe you need to use a bundler to have the scripts built as ESM/CJS, but I'm not sure.

@sysrage sysrage reopened this Nov 21, 2022
@ayushmanchhabra
Copy link
Collaborator

i'm using nw-builder inside a macos node js shell command "nwbuild".

If you're using it via a shell command, maybe the [nwbuild CLI command (https://github.com/nwutils/nw-builder/blob/3090addc3931c3ab5edea0e5afccfe613ed63b24/package.json#L28) may be a better fit.

since 4.0.0 var NwBuilder = require('nw-builder'); is not supported anymore (Error ERR_REQUIRE_ESM)

Apologies for this, oversight on my part. Temporary workaround would be using import()

a small porting guide from 3.x to 4.x would be nice

I'm working on documentation and adding a migration guide here but it far from complete.

what is the problem with the code?

I will try to reproduce on my end.

do the platforms need to be built separately now?

v4 nwbuild function focuses on doing one build. I find this to be more intuitive and easier to debug/test.

what about arm64 / universal on osx?

Officially only x64 build is supported right now. More info on that here: nwjs/nw.js#7620

@ayushmanchhabra
Copy link
Collaborator

Your fix for this isn't how ESM/async works. Example:

const { nwbuild } = require('nw-builder');

console.log('nwbuild', nwbuild);

You will see nwbuild undefined in the console, because the exported variable is undefined until the async IIFE in nwbuild.cjs resolves. Obviously, attempting to call nwbuild() also fails.

There are ways to support both CJS and ESM, but this isn't right. I believe you need to use a bundler to have the scripts built as ESM/CJS, but I'm not sure.

Yeah my bad I definitily I did this in haste without testing thoroughly. I'll look into the bundler solution.

@indexofrefraction
Copy link
Author

remark:
my settings above failed because using version:"latest" doesn't work !
with "0.70.1" the build starts (promise resolved)
the app doesn't run, there is no app.nw but at least something happens .-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Priority: 1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants