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

cannot directly use Node platform implementation #1

Closed
rijnhard opened this issue May 2, 2019 · 17 comments
Closed

cannot directly use Node platform implementation #1

rijnhard opened this issue May 2, 2019 · 17 comments

Comments

@rijnhard
Copy link

rijnhard commented May 2, 2019

for context: I have a library that I am using this in, and that library is linked globally while I test it in another application.

Env:

  • Node v10.15.3
  • OS Linux Mint 18.3

in the appropriate class in the library I have:

import { WeakRef, FinalizationGroup } from "tc39-weakrefs-shim/module/node";

Which then gets compiled bundled with babel7 and rollup.
Initially, I excluded the shim from babel transpilation and rollup bundling.

rollup.config.js snippet

const BABEL_CONFIG = babel({
    babelrc: true,
    exclude: 'node_modules/**',
    runtimeHelpers: true //transform-runtime plugin
}),
    RESOLVE_CONFIG = resolve({
        extensions: [ '.js', '.es', '.mjs', '.json' ]
    }),
    DEPENDENCIES = Object.keys(pkg.dependencies);

// excluding deep dependencies
DEPENDENCIES.push('tc39-weakrefs-shim/module/node');

export default [{
	input: [...],
	external: DEPENDENCIES,
	output: [
		{ dir: 'dist/common', format: 'cjs'},
		{ dir: 'dist/module', format: 'esm'}
	]
}];

which allowed rollup to bundle successfully, however now the application is throwing Syntax Errors

(function (exports, require, module, __filename, __dirname) { import { WeakTag, ObjectInfo } from "./weak-napi.js";
                                                                     ^

SyntaxError: Unexpected token {

I think it would be best to provide a commonjs compatible export, or how else would this work? rollup can handle dynamic imports and code splitting now so if you want esm and cjs support with dynamic imports in the bundle then I can probably do that setup quickly since thats how I run my libs.

@mhofman
Copy link
Owner

mhofman commented May 2, 2019

Yeah the problem is probably coming from the fact I need to import a node native module for the node implementation, which is currently done using a CommonJS file (node/weak-napi.js). They all have .js extensions which esm handled fine but other don't.

This package right now was mostly a proof of concept.
I had on my TODO to create CommonJS exports (compatible both browser and node), as well as make the ES Module exports more compatible, including with the Node 12 module work in progress.

I'll see what I can do. Any PR welcome to improve the situation.
In the meantime, maybe you can ask rollup to not transform only "module/node/weak-napi.js" ?

@ljharb
Copy link

ljharb commented May 2, 2019

ESM in node will have an .mjs extension; since it's still a flagged implementation, you'll want to transpile to ES5 CJS prepublish (consumers will use a bundler to consume it in a browser).

@mhofman
Copy link
Owner

mhofman commented May 2, 2019

Yeah, admittedly I was using this package as a playground to figure out a good way to publish both ESM and CJS, so that the ESM can be used directly in the browser without any bundlers or transpilers.

@ljharb
Copy link

ljharb commented May 2, 2019

You can still provide that kind of build, but the "main" in package.json needs to have node compatibility as a priority - ie, CJS ES5.

@mhofman
Copy link
Owner

mhofman commented May 2, 2019

100% agreed, hence why there is no main right now ;)
I just didn't work on that part yet.

@rijnhard
Copy link
Author

rijnhard commented May 2, 2019

Thanks guys: in the interim I have it working (so to spare others from going down the rollup rabbit hole and waking up in babylon)

I'm more than willing to setup babel and rollup, I'll make the PR tomorrow.
As a side note: are you looking at supporting this module officially? Because I want to use this in a fairly large project (since its the most futureproof of all the WeakRef libs).

rollup.config.js

import commonjs from 'rollup-plugin-commonjs';
import resolve from 'rollup-plugin-node-resolve';
import babel from 'rollup-plugin-babel';
import sourcemaps from 'rollup-plugin-sourcemaps';
import pkg from './package.json';

const BABEL_CONFIG = babel({
    babelrc: true,
    exclude: 'node_modules/**',
    include: [
        'src/',
        'node_modules/tc39-weakrefs-shim/module/node'
    ],
    runtimeHelpers: true //transform-runtime plugin
}),
    COMMONJS_CONFIG = commonjs({
        namedExports: {
            'node_modules/tc39-weakrefs-shim/module/node/weak-napi.js': ['WeakTag', 'ObjectInfo']
        },
        ignore: ['weak-napi']
    }),
    RESOLVE_CONFIG = resolve({
        extensions: [ '.js', '.es', '.mjs', '.json' ]
    }),
    DEPENDENCIES = Object.keys(pkg.dependencies);

export default [{
    input: [...],
	external: DEPENDENCIES,
	output: [
		{ dir: 'dist/common', format: 'cjs'},
		{ dir: 'dist/module', format: 'esm'}
	],
    plugins: [
		RESOLVE_CONFIG,
        COMMONJS_CONFIG,
        sourcemaps(),
        BABEL_CONFIG
    ]
}];

@mhofman
Copy link
Owner

mhofman commented May 2, 2019

Yeah I ended up extracting this from a bigger library I'm working on, so I'll keep supporting it for now.
If you look closely, the implementation is actually over-generic so that I can substitute some behavior and extend the WeakRef class.

I need to update it to the latest changes in the spec that happened in the past few days. No major changes, mostly around how host jobs are processed. I'm also not happy right now with the way the internal scheduling works.

@mhofman
Copy link
Owner

mhofman commented May 3, 2019

I just pushed a commit adding support for a CommonJS main.

I also changed the way I'm loading weak-napi to avoid using named exports from ESM.
The namedExports commonjs config workaround shouldn't be necessary anymore.
Once node-ffi-napi/weak-napi#15 is resolved, I believe this package will be directly usable as an ES Module.

I also took the opportunity to add support for the experimental modules support of node 12.
There is currently an issue with the esm package making it incompatible with "type": "module" in package.json. Once a release of esm with the fix is published, I'll remove the workaround.

Please let me know if this solves your issue, and I'll create a new release of the shim.
tc39-weakrefs-shim-0.0.1.tar.gz

@mhofman
Copy link
Owner

mhofman commented May 3, 2019

I extracted the weak-napi bindings into an external package so now the module output should be 100% isomorphic ESM.

You shouldn't need any more CommonJS workarounds and hopefully the module output is compatible with babel.
tc39-weakrefs-shim-0.0.1-2.tar.gz

@rijnhard
Copy link
Author

rijnhard commented May 4, 2019 via email

@rijnhard
Copy link
Author

rijnhard commented May 6, 2019

I extracted the weak-napi bindings into an external package so now the module output should be 100% isomorphic ESM.

You shouldn't need any more CommonJS workarounds and hopefully the module output is compatible with babel.
tc39-weakrefs-shim-0.0.1-2.tar.gz

Gave it a test, it now builds in rollup without any special settings, other than having the commonjs included where I didn't previously need it.

import commonjs from 'rollup-plugin-commonjs';
import resolve from 'rollup-plugin-node-resolve';
import babel from 'rollup-plugin-babel';
import sourcemaps from 'rollup-plugin-sourcemaps';
import pkg from './package.json';

const BABEL_CONFIG = babel({
    babelrc: true,
    exclude: 'node_modules/**',
    /* no special includes needed */ 
    runtimeHelpers: true //transform-runtime plugin
}),
    COMMONJS_CONFIG = commonjs({ /* no settings needed */ }),
    RESOLVE_CONFIG = resolve({
        extensions: [ '.js', '.es', '.mjs', '.json' ]
    }),
    DEPENDENCIES = Object.keys(pkg.dependencies);

export default [{
    input: [...],
	external: DEPENDENCIES,
	output: [
		{ dir: 'dist/common', format: 'cjs'},
		{ dir: 'dist/module', format: 'esm'}
	],
    plugins: [
		RESOLVE_CONFIG,
        COMMONJS_CONFIG, // this is still needed
        sourcemaps(),
        BABEL_CONFIG
    ]
}];

@rijnhard
Copy link
Author

rijnhard commented May 6, 2019

nvm, fixed it, forgot since I'm using the Node binding directly that I need to manually add the dependency to the external

import resolve from 'rollup-plugin-node-resolve';
import babel from 'rollup-plugin-babel';
import sourcemaps from 'rollup-plugin-sourcemaps';
import pkg from './package.json';

const BABEL_CONFIG = babel({
    babelrc: true,
    exclude: 'node_modules/**',
    runtimeHelpers: true //transform-runtime plugin
}),
    RESOLVE_CONFIG = resolve({
        extensions: [ '.js', '.es', '.mjs', '.json' ]
    }),
    DEPENDENCIES = Object.keys(pkg.dependencies);

DEPENDENCIES.push('tc39-weakrefs-shim/module/node');

export default [{
    input: [...],
	external: DEPENDENCIES,
	output: [
		{ dir: 'dist/common', format: 'cjs'},
		{ dir: 'dist/module', format: 'esm'}
	],
    plugins: [
		RESOLVE_CONFIG,
        sourcemaps(),
        BABEL_CONFIG
    ]
}];

this is about as standard as you can get. so I would say this issue can be closed, and if you are happy then publish a new release (I've built from master)

@mhofman
Copy link
Owner

mhofman commented May 6, 2019

Yeah unfortunately it looks like rollup-plugin-node-resolve doesn't yet support package.json's "type": "module" field.
It looks like rollup supports .mjs files though, but I'd rather not use that extension, as to make ESM isomorphic you need to include the extensions in the import statement, which would mean .mjs in the browser.

Since dual mode packages are not officially supported yet, I think we just need to wait for things to settle and tooling to catch up.

EDIT: My bad, rollup doesn't actually care about file types. Any suggestion on how to improve the deep linking story?

You should be able to use "tc39-weakrefs-shim/lib/node" if you want to stick to commonjs.
I'll update the README and publish a new version soon.

@ljharb
Copy link

ljharb commented May 6, 2019

.mjs in the browser works fine, since browsers don't care about extensions - true isomorphic code does use .mjs in the browser, since that's what will parse as a Module in node.

@mhofman
Copy link
Owner

mhofman commented May 6, 2019

.mjs in the browser works fine, since browsers don't care about extensions - true isomorphic code does use .mjs in the browser, since that's what will parse as a Module in node.

Renaming everything would be an extensive change to my projects right now, which I'd rather avoid.
It seems like the extension is not the problem anyway, but more the deep linking.

Also, "type": "module" will parse in node as well, I've actually made sure the package runs on Node 12 experimental.
For now I'll keep the module support as-is, especially since I'm now supporting a commonjs main as well.

@mhofman
Copy link
Owner

mhofman commented May 6, 2019

Version 0.0.2 has been published.

@mhofman mhofman closed this as completed May 6, 2019
@rijnhard
Copy link
Author

rijnhard commented May 9, 2019

Just for good measure - confirming it works with "tc39-weakrefs-shim/lib/node" even with odd npm link configurations

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