Fixed incorrect build:browser script#56
Conversation
- Updated to build for browser, not node - Simplified imports - Updated and build-output.test.js - Removed redundant Jest config
|
@springmeyer this is a PR to resolve the |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the incorrect platform target for the build:browser script and modernizes the test imports by replacing TypeScript path aliases with relative imports.
- Updated the browser build script to generate an IIFE bundle for browser use instead of CommonJS for Node.js
- Replaced all TypeScript path aliases (
@/) with relative imports (../src) in test files - Enhanced build output tests with direct checks of the generated browser bundle
Reviewed Changes
Copilot reviewed 13 out of 42 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Fixed build:browser script to use IIFE format for browser compatibility |
| arc.js | Generated browser bundle now uses IIFE format with global arc variable |
| src/line-string.ts | Renamed LineString class to _LineString to indicate internal use |
| src/index.ts | Added export for _LineString class |
| src/arc.ts | Updated import to use renamed _LineString class |
| src/great-circle.ts | Updated import to use renamed _LineString class |
| test/*.ts | Replaced TypeScript path aliases with relative imports |
| test/build-output.test.js | Enhanced tests with browser bundle validation and ESM testing |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| test('Browser bundle (arc.js) exists', () => { | ||
| const fs = require('fs'); | ||
| const path = require('path'); |
There was a problem hiding this comment.
Move the require statements to the top of the file to follow Node.js best practices and improve readability.
|
|
||
| // Test API completeness | ||
| test('All original API methods work', () => { | ||
| const arc = require('../dist/index.js'); |
There was a problem hiding this comment.
Move the require statement to the top of the file to follow Node.js best practices and improve readability.
| const arcBundle = require('../arc.js'); | ||
| // Test output consistency between CommonJS and ESM | ||
| test('Output consistency between CommonJS and ESM', () => { | ||
| const arcCJS = require('../dist/index.js'); |
There was a problem hiding this comment.
Move the require statement to the top of the file to follow Node.js best practices and improve readability.
This PR primarily fixes the incorrect platform target for the
build:browserscript as discussed in this comment.This PR bring in the following changes:
build:browserto build./arc.jsfor browser use, not node.js,./buildfolder and re-ran all tests,./test/build-output.test.jswith direct checks in the built files, and