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

feat(puppeteer): export esm modules in package.json #7964

Merged
merged 6 commits into from Feb 9, 2022
Merged

Conversation

jrandolf-zz
Copy link
Contributor

@jrandolf-zz jrandolf-zz commented Feb 4, 2022

Fixes #6753

@OrKoN
Copy link
Collaborator

OrKoN commented Feb 7, 2022

I get the following error if I try to use ESM version for a NodeJS package:

/Users/alexrudenko/src/puppeteer/lib/esm/puppeteer/node.js:16
import { initializePuppeteerNode } from './initialize-node.js';
^^^^^^

SyntaxError: Cannot use import statement outside a module
    at Object.compileFunction (node:vm:352:18)
    at wrapSafe (node:internal/modules/cjs/loader:1031:15)
    at Module._compile (node:internal/modules/cjs/loader:1065:27)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/translators:190:29)
    at ModuleJob.run (node:internal/modules/esm/module_job:185:25)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:281:24)

The code for the test module is:

import puppeteer from 'puppeteer';

(async () => {
  const browser = await puppeteer.launch();
  const page = await browser.newPage();
  await page.goto('http://example.com');
  await page.screenshot({ path: 'example.png' });
  await browser.close();
})();

@mathiasbynens
Copy link
Member

@OrKoN What's the filename of this test module? It either needs to have the .mjs extension (easiest) or you need to configure things in its package.json.

@OrKoN
Copy link
Collaborator

OrKoN commented Feb 7, 2022

@mathiasbynens it has type=module in package.json

src/initialize-node.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@jrandolf-zz jrandolf-zz force-pushed the ship_esm branch 3 times, most recently from e0ff1dc to 35e934b Compare February 7, 2022 22:07
Signed-off-by: Randolf Jung <jrandolf@chromium.org>
@jrandolf-zz jrandolf-zz force-pushed the ship_esm branch 2 times, most recently from a75262c to ae6ad1f Compare February 8, 2022 02:02
package.json Outdated Show resolved Hide resolved
docs/api.md Outdated Show resolved Hide resolved
@jrandolf-zz jrandolf-zz enabled auto-merge (squash) February 8, 2022 13:36
@jrandolf-zz jrandolf-zz force-pushed the ship_esm branch 5 times, most recently from 242452f to 67a4486 Compare February 9, 2022 06:44
compat/esm/compat.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@OrKoN OrKoN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! nice work on the compatibility layer!

@jrandolf-zz jrandolf-zz merged commit 523b487 into main Feb 9, 2022
@jrandolf-zz jrandolf-zz deleted the ship_esm branch February 9, 2022 07:47
@mathiasbynens
Copy link
Member

Nice work! Should we add some documentation to the README?

@my-lalex
Copy link

my-lalex commented Feb 10, 2022

I didn't investigate further, but the join(puppeteerDirname, 'package.json') from src/node/NodeWebSocketTransport.ts gets the wrong path to get the package.json in my setup (typescript, webpack): it return a path which is one level too high.

The relative path from '13.2' seems to work ../../../../package.json as downgrading worked for me...

@jrandolf-zz
Copy link
Contributor Author

jrandolf-zz commented Feb 10, 2022

@my-lalex Since you are using webpack, I assume you have everything output to a single file. Could you compare the output with the previous output? Specifically the area where puppeteerDirname is used.

@my-lalex
Copy link

I kinda find the same as in the code source.... maybe using the __dirname variable is not ideal, some issues are known to happen when using it with bundlers.... Why not using resolve.require() to find the puppeteer path?

Another point is that if all the code is bundled (including puppetteer's) - let's say for offline environment- there's no 'package.json' anymore.... wouldn't it be a good idea to store the version number in a global variable and use it after that?
To avoid updateing the variable value at each release, it could be done by importing the package.json in the typescript code and using the resolveJsonModule options of tsconfig... and it would prevent a JSON parsing at runtime only for a version number...

@jrandolf-zz
Copy link
Contributor Author

I am not sure what you mean by "offline environment". The code in node is expected to be used in a Node environment. Moreover, I believe resolveJsonModule only helps resolve the path. Not parsing it at compile time and embedding it. Since code can be externalized in Webpack, for now that would be our recommendation.

@my-lalex
Copy link

my-lalex commented Feb 10, 2022

When saying "offline environment", I'm talking about a computer having no internet connection. When this is that kind of target, you cannot use "npm install" to get your packages before running. So you (can) bundle all your code in a single file as in Node file size is less important.... and this way you just have one file to deploy, getting rid of all the node_modules folder... and specify a executablePath option

My bad about resolveJsonModule: bundlers include the JSON content in the code but it's not a TS feature....

A global variable, updated by the building process (or manually), would then be a good solution... for my part, I think that parsing a file (which might not be there) each time you need the package version isn't an optimal solution...

And if this has to be the solution by deciding that puppeteer cannot be bundled, the use of require.resolve would seem to me as being the most viable solution for every building solution... (actually, I already use it with puppeteer to get the full path of the chromium binary)

let puppeteerPath = path.join(__dirname, "node_modules", "puppeteer");
// Just to be sure, depends on the environment
try {
    puppeteerPath = path.dirname(require.resolve('puppeteer'));
} catch (e) {}

OrKoN added a commit that referenced this pull request Feb 10, 2022
jrandolf-zz pushed a commit that referenced this pull request Feb 10, 2022
* Revert "fix(puppeteer): export internals (#7991)"

This reverts commit 448118c.

* Revert "feat(puppeteer): export esm modules in package.json (#7964)"

This reverts commit 523b487.
This was referenced May 30, 2022
This was referenced May 30, 2022
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

Successfully merging this pull request may close these issues.

Ship Puppeteer as JavaScript modules
5 participants