4.0 build system #911

patrickarlt · 2021-09-07T20:33:31Z

This is a major update and modernization of the build system to resolve a whole slew of things:

Drop lerna for NPM workspaces #908
Add support for ESM modules in Node #907
Remove requirement to manually include fetch and FormData in Node #906
drop support for Internet Explorer and Edge Legacy #785
Build ES modules for public CDN #788
Removes support for custom fetch (i.e. IRequestOptions.fetch)
Merge request and auth modules Reduce peerDependencies #786

Currently the work is only done for arcgis-rest-request. This is because we need to add .js to EVERY file extension to #907 since the module spec requires it. The Node JS tests require this. Once this gets general approval I can go through and add .js everywhere else.

I'll try to summarize the changes here and with inline comments:

This is ~50% though the process of (Drop lerna for NPM workspaces #908). There are still some issues:
- npm version --ws Does not update the dependencies or peerDependencies in the packages like lerna does. We either have to make a custom script that does this OR use lerna just for this one command or switch to standard-version with some custom processing to also get the changelog tools. I'm open for discussion on this.
- None of the release process script that use lerna have been updated yet.
The prepare lifecycle script has been removed from all packages. This should probably get re-added before this gets merged. Or a global "prepare": "npm run build -ws" in the root package.json.
Test matrix expanded to the latest Node 12, 14 and , 16.
Adds 2 new demos for ES Modules and Common JS modules in Node
Removes support for custom fetch in IRequestOptions. This greatly simplifies just about everything (mostly getting TypeScript checks to pass) These tests have been commented out for now.
arcgis-rest-request now has a dependency on formdata-node which is an isomorphic FormData ponyfill for Node and @esri/arcigs-rest-request-fetch which is basically cross-fetch without a browser ponyfill. This remove the requirement for Node users to provide their own fetch and FormData polyfills.
To further avoid side effects and allow tree shaking setDefaultRequestOptions now sets DEFAULT_ARCGIS_REQUEST_OPTIONS on globalThis. A new getDefaultRequestOptions fetches the defaults. This might avoid an edge case in which let DEFAULT_ARCGIS_REQUEST_OPTIONS = {} got tree shaken out. See https://sgom.es/posts/2020-06-15-everything-you-never-wanted-to-know-about-side-effects/
Adds a engine property in package.json set to >=12.16.0, this is when conditional exports was unflagged https://nodejs.org/api/packages.html#packages_conditional_exports
The build process now works like:
- Runs TypeScript with no output to check for errors
- Complies to CommonJS dist/cjs
- Compiles to JavaScript Modules dist/esm
- Compiles to UMD dist/umd
- Creates package.jsons in dist/cjs and dist/esm to markt he correct module type for Node.
- Runs the Skypack package checker to make sure the package.json looks good for the Skypack CDN which will resolve Build ES modules for public CDN #788.

In order to merge this:

patrickarlt · 2021-09-08T15:49:49Z

Note that there is some risk in the dual CJS/EMS approach, mainly:

When an application is using a package that provides both CommonJS and ES module sources, there is a risk of certain bugs if both versions of the package get loaded. This potential comes from the fact that the pkgInstance created by const pkgInstance = require('pkg') is not the same as the pkgInstance created by import pkgInstance from 'pkg' (or an alternative main path like 'pkg/module'). This is the “dual package hazard,” where two versions of the same package can be loaded within the same runtime environment. While it is unlikely that an application or package would intentionally load both versions directly, it is common for an application to load one version while a dependency of the application loads the other version. This hazard can happen because Node.js supports intermixing CommonJS and ES modules, and can lead to unexpected behavior.

https://nodejs.org/api/packages.html#packages_dual_package_hazard

There are a few mitigating approaches to this mainly Approach #2: Isolate state

This can be done if both the CommonJS and ES module versions of the package are equivalent, for example because one is the transpiled output of the other; and the package’s management of state is carefully isolated (or the package is stateless).

In general though I think we are OK since the entire library is stateless anyway. The only state we MIGHT use is DEFAULT_ARCGIS_REQUEST_OPTIONS which now exists on the global object if you call setDefaultRequestOptions() or state that the application is supposed to instantiate like UserSesssion

This might be a concern though for things like hub.js or solutions.js though @dbouwman @tomwayson.

patrickarlt · 2021-09-08T17:42:16Z

Additional consideration is this comment in the node-fetch example of the formdata-node repo:

Note that node-fetch >= 3.x have builtin support for spec-compliant FormData, sou you'll only need the form-data-encoder if you use node-fetch <= 2.x.

Since we use [email protected] we might run into this although I question if "spec-compliant" is really necessary for our various use cases. I would REALLY appreciate someone else testing this with stuff like addResources or addAttchment with a practical tests case in Node since I am really not familiar with them. If we find issues I think we can work around this.

patrickarlt · 2021-09-13T21:37:29Z

All demos have been updated and are working. However demos/webmap-checker-sapper probably needs to be rewritten since sapper is now sveltekit and has had a top-bottom reinventing.

patrickarlt · 2021-09-14T02:16:51Z

More additions/fixes

Switched to the import type/export type format when using types from external packages. This makes bundles smaller and prevents some Rollup errors.
Added a test:compile to confirm that everything compiles with TypeScript before running other tests.
Added tooling to format package.jsons on commit and as an npm command. This makes it WAY easier to compare changes to package.jsons accross the repo.
New browser-es-modules demo showing native ES modules in the browser with an importmap, polyfilled for FF and Safari. We can use this as a base for future demos.
New snowpack demo for Snowpack
New parcel demo for Parcel
New vite demo for Vite
We also now export package.json from the node exports to avoid package.json is not defined by "exports" uuidjs/uuid#444 (comment)
We now inline all file content in the sourcemaps. Parcel complained about this.
Rollup config has now moved to a single rollup.js config file that generates 4 builds:
- {packge}.umd.js - UMD for people who need it.
- {packge}.umd.min.js - Minified UMD for people who need it.
- {packge}.esm.js - Native JS module with @esri/arcgis-rest-fetch and @esri/arcgis-rest-form-data bundled. Mostly for our own demos and people who want to use ArcGIS REST JS via script tags with import maps WITHOUT Skypack.
- {packge}.esm.min.js - Above but minified

Commands in the package.jsons accross the repo have been re-worked some use ultra-runner and some use npm-run-all to try to get the fastest build times:

In the root:

For all packages

npm run build - builds all distributions of every package with ultra, inside each package builds are done in parallel with npm-run-all. Output is errors only.
npm run build:esm, npm run build:cjs, npm run build:bundled - as as the above but only one of our target distributions.
npm run dev:esm, npm run dev:cjs, npm run dev:bundled - runs the appropriate watch command in all packages.

For a specific package

npm run build -w @esri/arcgis-rest-request - run all builds in a specific workspace
npm run dev -w @esri/arcgis-rest-request - run all dev commands in a specific workspace
npm run build:esm -w @esri/arcgis-rest-request - run the esm build in a specific workspace
npm run dev:esm -w @esri/arcgis-rest-request - run the esm dev command in a specific workspace
npm run build:cjs -w @esri/arcgis-rest-request - run the common js build in a specific workspace
npm run dev:cjs -w @esri/arcgis-rest-request - run the common js dev command in a specific workspace
npm run build:bundled -w @esri/arcgis-rest-request - run the rollup build in a specific workspace
npm run dev:bundled -w @esri/arcgis-rest-request - run the rollup dev command in a specific workspace

…n tests

codecov · 2021-09-14T22:58:32Z

Codecov Report

❗ No coverage uploaded for pull request base (v4.0@2a6e355). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff            @@
##             v4.0      #911   +/-   ##
========================================
  Coverage        ?   100.00%           
========================================
  Files           ?       139           
  Lines           ?      2380           
  Branches        ?       413           
========================================
  Hits            ?      2380           
  Misses          ?         0           
  Partials        ?         0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a6e355...d2af66f. Read the comment docs.

patrickarlt · 2021-09-15T21:11:48Z

PR summary for review:

Breaking Changes

Remove support for IE 11 and Legacy Edge. We now compile for ES 2017 as a target however we are not currently using any ES 2017 features. In General this means:
- Modern versions (last 2) of Chrome, Firefox, Edge, Safari on desktop and mobile devices. Many other versions/browsers will work as long as they support ES 2017, fetch, FormData and Promise.
- Node 12.20+
fetch is now bundled for Node JS via node-fetch.
FormData is now bundled for Node JS via formdata-node. Since formdata-node supplies only a standards compliant FormData implementation steams and buffers are no longer supported as parameters. You will need to use the File or Blob classes from formdata-node, @esri/arcgis-rest-form-data or @esri/arcgis-rest-request.
In @esri/arcgis-rest-portal the addItemData(), createItem(), updateItem(), updateItemInfo() and createItemInFolder() no longer accept a data parameter. Use text or file as documented. data was previously used in some tests.
In @esri/arcgis-rest-portal the The file option in addItemData(), createItem(), updateItem(), updateItemInfo() and createItemInFolder() and now needs to be a File or Blob object. See the attachments-node demo for how to work with the File and Blob classes in Node. In browsers this is likely already a File object from an <input type="File">.
In @esri/arcgis-rest-feature-layer the The attachment option in addAttachment(), updateAttachment(), and now needs to be a File or Blob object. See the attachments-node demo for how to work with the File and Blob classes in Node. In browsers this is likely already a File object from an <input type="File">.
Support for supplying a custom fetch implementation in request options has been removed.
@esri/arcgis-request-auth has been merged into @esri/arcgis-rest-request.

Additions/Changes

Node JS users no longer need to supply fetch and FormData polyfills. Node JS is likely to add support for a native fetch and possibly FormData in a future version. Bundling our own polyfills helps prepare us for that future.
Native support for ES modules in Node JS. Node supports ES modules unflagged since 12.17.0.See the Modules: ECMAScript modules documentation.
New bundled builds for supporting ES modules in browsers.

Notes

As of v4.0 we will also support ES modules via the Skypack CDN for developers that want to use ES modules in the browser without a build tool. This is already possible but we shoud document and support it.
I also can get Deno working locally in the repo. Users should use Skypack but I have a working demo of it I can push up if any one feels like it should be included.
I also tested React native and this works. This probably worked in previous versions but it definitely works now.

noahmulfinger

I went through some of the code and some demos and I have a few comments. This could all be work done separately on the v4 branch though if we want to just get this PR in first.

Linting-wise, I notice there are a few files that have unused imports. Is the linter not catching that?
- packages/arcgis-rest-portal/src/items/add.ts
- packages/arcgis-rest-portal/src/items/helpers.ts
- packages/arcgis-rest-request/src/federation-utils.ts
Some of the demos have package-lock.json files. Should those exist since we are using an npm workspace?
The browser-es-modules demo has some errors, like "request is not defined". It also could use a little clean up because it looks like the JSON is being displayed as HTML.
The node-es-modules and node-common-js demos are missing start scripts.
The snowpack and vite demo ask users to run npm install, but that creates a package-lock.json file that isn't gitignored.

.github/workflows/pre-release-tests.yml

noahmulfinger · 2021-09-16T18:49:15Z

packages/arcgis-rest-auth/src/index.ts

-export * from "./app-tokens";
-
-export * from "./validate-app-access";
+export {


It looks like we are re-exporting here for backwards compatibility? Do we need to do this since we are created a new release with breaking changes? Seems like we could just remove the arcgis-rest-auth folder altogether.

@noahmulfinger this is the major breaking change from 3.x > 4.x we are mostly doing this to that we don't have to immediately rewrite all tutorials and sections in the developer guide.

noahmulfinger · 2021-09-16T20:44:34Z

package.json

+    ],
+    "*.ts": [
+      "prettier --write --parser typescript --tab-width 2 --use-tabs false",
+      "eslint --project tsconfig.json",


Looks like we switched away from tslint. Any reason? Also, it means we can probably remove all tslint:disable comments.

TSLint was deprecated in 2019 https://palantir.github.io/tslint/ and merged with eslint. I there are only a few tslint:disable comments all in the tests which I think are still respected by the parser.

package.json

gavinr · 2021-09-17T16:04:28Z

I ran npm build and npm run test in my environment, it looks good there.

In terms of documentation updates, I'd like to see (And am happy to try to help contribute):

Update README.md - contains a reference to lerna, which this PR removes (I think), and is also probably generally outdated after this major update and probably needs a refresh.
- I'm not sure we've ever had this, but it would be nice to add a "developer quick start" - like if I'm a developer not super familiar with the project, how do I get a quick dev environment going if I want to do a fix on a particular package (For example, making a change to the routing package)
docs\HISTORY.md - at minimum, "Architecture" needs update (or removal?) ... and maybe a general update to this file with new status.
RELEASE.md - If the plan is to update the release workflow after we merge this PR, maybe this will be updated then.
docs\src\guides\node.md needs updating, possibly other pages in the guide section as well.

patrickarlt · 2021-09-24T23:26:43Z

I think this is ready to merge. I addressed #911 (comment) and #911 (review) and responded to @noahmulfinger's comments in #911 (comment) and #911 (comment)

tomwayson · 2021-09-27T18:00:40Z

tomwayson

Heroic work @patrickarlt, let's get this merged!

patrickarlt added 16 commits September 5, 2021 08:51

4.0 build system WIP

1f3be28

merge

a4840ff

working build system

4358363

wip

6c4a91d

co

3f021a0

add fetch package

cd5d7b7

wip

1503252

remove typedoc

3a88f72

more cleanup

6b85c69

remove package locks

063617b

restore typedoc

2050f8e

continue cleanup

c094402

remove old ts-helper for node tests

27cfeb9

remove old ts-helper for node tests

b666f63

update engine

09cf5cc

delete lerna.json

f3b61dd

patrickarlt requested review from dbouwman, gavinr, tomwayson, noahmulfinger and araedavis September 7, 2021 20:33

continue cleanup

37cdcda

patrickarlt added this to the v4.0 milestone Sep 7, 2021

This was referenced Sep 8, 2021

Switch to npm local paths #873

Closed

"npm run dev" fails on Windows #741

Closed

should we ditch the changelog tooling? #688

Closed

Trouble polyfill fetch in Node with typescript #666

Closed

patrickarlt changed the base branch from master to v4.0 September 8, 2021 17:01

patrickarlt added 3 commits September 13, 2021 17:27

more demos

792fe3c

use import/export type, ultra runner, better rollup

8435faa

format package.jsons

cd0ab49

patrickarlt marked this pull request as ready for review September 14, 2021 02:20

patrickarlt added 7 commits September 13, 2021 19:20

update actions

6c23e01

fix build script

62c9780

fix tests, add file attachment node demo, remove data for file/text i…

766ed16

…n tests

use npm 7 in GitHub actions

1936f6a

fix build

dd0a99c

upgrade npm in actions

0cd5959

fix tests

cfc76cb

patrickarlt added 4 commits September 14, 2021 16:03

don't fail fast on tests

8459553

don't fail fast on tests

2594b60

sass

a6f820c

fix tests

b194a5c

noahmulfinger changed the title ~~DRAFT: 4.0 build system~~ 4.0 build system Sep 16, 2021

noahmulfinger reviewed Sep 16, 2021

View reviewed changes

patrickarlt added 2 commits September 24, 2021 15:57

deno demo, cleanup

24ba0a8

final cleanup

d1863fc

fix tests in windows, cleanup test deprecations

d2af66f

gavinr approved these changes Sep 28, 2021

View reviewed changes

tomwayson approved these changes Oct 1, 2021

View reviewed changes

patrickarlt merged commit e8d1be1 into v4.0 Oct 4, 2021

4.0 build system #911

4.0 build system #911

Uh oh!

Conversation

patrickarlt commented Sep 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patrickarlt commented Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patrickarlt commented Sep 8, 2021

Uh oh!

patrickarlt commented Sep 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patrickarlt commented Sep 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

patrickarlt commented Sep 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking Changes

Additions/Changes

Notes

Uh oh!

noahmulfinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

noahmulfinger Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickarlt Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

noahmulfinger Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickarlt Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gavinr commented Sep 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patrickarlt commented Sep 24, 2021

Uh oh!

tomwayson commented Sep 27, 2021 via email

Uh oh!

tomwayson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

patrickarlt commented Sep 7, 2021 •

edited

Loading

patrickarlt commented Sep 8, 2021 •

edited

Loading

patrickarlt commented Sep 13, 2021 •

edited

Loading

patrickarlt commented Sep 14, 2021 •

edited

Loading

codecov bot commented Sep 14, 2021 •

edited

Loading

patrickarlt commented Sep 15, 2021 •

edited

Loading

noahmulfinger Sep 16, 2021 •

edited

Loading

noahmulfinger Sep 16, 2021 •

edited

Loading

gavinr commented Sep 17, 2021 •

edited

Loading