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

chore: Move source files into src/ and emit to lib/ with TypeScript #5568

Merged
merged 1 commit into from Apr 2, 2020

Conversation

jackfranklin
Copy link
Collaborator

@jackfranklin jackfranklin commented Apr 1, 2020

This updates our tsconfig.json so it emits our JavaScript files as
well as type checking them. We compile into ./out which we then ship
in our npm package.

Because the lib/ directory is exclusively JS files, this change is a
no-op in terms of code functionality but is the first step towards being
able to replace lib/X.js with lib/X.ts in a way that allows us to
migrate incrementally.

The out directory is gitignored, and the lib directory is
npmignored. On npm publish we will now run npm run tsc in order to
generate the outputted code.

Any imports that import from lib/X now import from out/X.

BREAKING CHANGE: if users are importing Puppeteer modules, e.g.:

const PuppeteerWorker = require('puppeteer/lib/Worker');

This path has now changed to:

const PuppeteerWorker = require('puppeteer/out/Worker');

** EDIT ** the above is outdated; the below is what we now do:

This updates our tsconfig.json so it emits our JavaScript files as
well as type checking them. We compile into ./lib which we then ship
in our npm package. The source code has moved from ./lib into ./src.

Because the src/ directory is exclusively JS files, this change is a
no-op in terms of code functionality but is the first step towards being
able to replace src/X.js with src/X.ts in a way that allows us to
migrate incrementally.

The lib directory is gitignored, and the src directory is
npmignored. On npm publish we will now run npm run tsc in order to
generate the outputted code.

@jackfranklin
Copy link
Collaborator Author

@mathiasbynens @TimvdLippe would love your thoughts on this please.

The goal here is to have TypeScript not only type-check but emit our JavaScript, in preparation for us to start replacing files with *.ts ones.

This PR is more a starting point & one solution that works rather than definitely the best one - I think there's a few ways we could do this and I'm happy to chat about it. Can always jump on a call :)

@jackfranklin
Copy link
Collaborator Author

jackfranklin commented Apr 1, 2020

CI is failing for a legitimate reason: node install.js needs out/helper.js, but that can't exist until TypeScript has run. (It depends on it because node install.js requires the main Puppeteer module)

However for TypeScript to run it needs protocol.d.ts, which is installed by node install.js. So I think we need to either:

  • Update the install script to not depend on out/helper
  • Update the install script to depend on lib/helper.js (works until we want to TypeScriptify that file)
  • Update the install script to grab protocol.d.ts, then run TypeScript, then run the install script. This seems wasteful given this runs every time anyone installs the package from npm.

EDIT: thinking about it, this isn't a problem for an end user installing the package because out/ will already exist. So this is a problem for CI runs and installing the package to work on locally.

@TimvdLippe
Copy link
Contributor

TimvdLippe commented Apr 1, 2020

GitHub search is not really working for me, but this search seemed the most sane: https://github.com/search?q=%22puppeteer%2Flib%22&type=Code That search isn't immediately showing any actual usage of direct files in puppeteer.

Emitting with tsc seems 👍 One option you also have is to move lib/ into src/ and then still write to lib/. That might not be a breaking change after all?

I have no knowledge about the install scripts and how they all tie together. I will leave that to @mathiasbynens

@mathiasbynens
Copy link
Member

We can prevent the breaking change in this case, so let's do it. I like Tim's suggestion of moving lib to src and then having lib be the generated output. I also appreciate you sending this smaller patch first, so that it's easier to review the actual changes 👍

@jackfranklin
Copy link
Collaborator Author

Great thinking - I'll update this PR to move lib/X into src/X and generate into lib.

@jackfranklin jackfranklin changed the title chore: Emit lib/ to out/ with TypeScript chore: Move source files into src/ and emit to lib/ with TypeScript Apr 2, 2020
package.json Outdated Show resolved Hide resolved
This updates our `tsconfig.json` so it emits our JavaScript files as
well as type checking them. We compile into `./lib` which we then ship
in our npm package. The source code has moved from `./lib` into `./src`.

Because the `src/` directory is exclusively JS files, this change is a
no-op in terms of code functionality but is the first step towards being
able to replace `src/X.js` with `src/X.ts` in a way that allows us to
migrate incrementally.

The `lib` directory is gitignored, and the `src` directory is
npmignored. On `npm publish` we will now run `npm run tsc` in order to
generate the outputted code.
@mathiasbynens mathiasbynens merged commit 7a2a41f into master Apr 2, 2020
@mathiasbynens mathiasbynens deleted the emit-with-typescript branch April 2, 2020 14:25
@mathiasbynens mathiasbynens added this to the TypeScript migration milestone Apr 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants