Revamp builds #800
Revamp builds #800
Conversation
| // esm & cjs | ||
| const inputs = [ | ||
| 'tom-select.ts', | ||
| 'tom-select.complete.ts', | ||
| 'tom-select.popular.ts', | ||
| 'utils.ts', | ||
| ]; |
There was a problem hiding this comment.
These builds are now handled by tsc.
| @@ -128,7 +84,7 @@ function configCore( input, filename, plugins ){ | |||
|
|
|||
| var output = { | |||
| name: 'TomSelect', | |||
| file: `build/js/${filename}`, | |||
| file: `dist/js/${filename}`, | |||
There was a problem hiding this comment.
AFAICT the build/dist distinction wasn't useful. I opted to just directly construct everything in dist, which simplified things a bit.
| import TomSelect from 'tom-select/base'; | ||
| import TomSelect_remove_button from 'tom-select/plugins/remove_button.js'; | ||
| import TomSelect_dropdown_header from 'tom-select/dropdown_header.js'; |
There was a problem hiding this comment.
IMO this is cleaner and transparently handles ESM vs. CJS. This is enabled by the new exports in package.json.
| "./base": { | ||
| "import": "./dist/esm/tom-select.js", | ||
| "require": "./dist/cjs/tom-select.js" | ||
| }, | ||
| "./popular": { | ||
| "import": "./dist/esm/tom-select.popular.js", | ||
| "require": "./dist/cjs/tom-select.popular.js" | ||
| }, | ||
| "./utils": { | ||
| "import": "./dist/esm/utils.js", | ||
| "require": "./dist/cjs/utils.js" | ||
| }, | ||
| "./plugins/*": { | ||
| "import": "./dist/esm/plugins/*", | ||
| "require": "./dist/cjs/plugins/*" | ||
| }, |
There was a problem hiding this comment.
There are new; this felt nicer than manually reaching into dist.
| @@ -6,7 +6,7 @@ | |||
| * - Modified by Brian Reavis <brian@thirdroute.com> 2012-8-27 (cleanup) | |||
| */ | |||
|
|
|||
| import {replaceNode} from '../vanilla'; | |||
| import {replaceNode} from '../vanilla.ts'; | |||
There was a problem hiding this comment.
These extensions make the ESM build work properly. tsc will transform these appropriately during build.
| /** | ||
| * Iterates over arrays and hashes. | ||
| * | ||
| * ``` | ||
| * iterate(this.items, function(item, id) { | ||
| * // invoked for each item | ||
| * }); | ||
| * ``` | ||
| * | ||
| */ | ||
| export const iterate = (object:[]|{[key:string]:any}, callback:(value:any,key:any)=>any) => { | ||
|
|
||
| if ( Array.isArray(object)) { | ||
| object.forEach(callback); | ||
|
|
||
| }else{ | ||
|
|
||
| for (var key in object) { | ||
| if (object.hasOwnProperty(key)) { | ||
| callback(object[key], key); | ||
| } | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
I opted to inline this instead of reaching into @orchidjs/sifter.
| @@ -1,7 +1,6 @@ | |||
| import {nodeResolve} from '@rollup/plugin-node-resolve'; // so Rollup can resolve imports without file extensions and `node_modules` | |||
| import babel from '@rollup/plugin-babel'; | |||
| import terser from '@rollup/plugin-terser'; | |||
| import pkg from '../package.json' assert { type: "json" }; | |||
There was a problem hiding this comment.
This wasn't compatible with Node 18, which is now part of the test matrix. It's possible that we don't care at all about multiple Node versions since this is all meant to run in the browser.
| @@ -89,7 +90,7 @@ export default class TomSelect extends MicroPlugin(MicroEvent){ | |||
| public userOptions : {[key:string]:boolean} = {}; | |||
| public items : string[] = []; | |||
|
|
|||
| private refreshTimeout : null|ReturnType<typeof setTimeout> = null; | |||
| private refreshTimeout : null|number = null; | |||
There was a problem hiding this comment.
ReturnType<typeof setTimeout> was transformed to NodeJS.Timeout during the build process. This resulted in an error when I tried using this new version in my project:
../../node_modules/tom-select/dist/esm/utils.d.ts:27:68 - error TS2503: Cannot find namespace 'NodeJS'.
27 export declare const timeout: (fn: () => void, timeout: number) => NodeJS.Timeout | null;
~~~~~~
Found 1 error.
Since this should always run in the browser, it's safe to use number here. See corresponding change in src/utils.ts from setTimeout to window.setTimeout, and the change in onInput below from clearTimeout to window.clearTimeout.
|
I've tested this in my own project and it's working correctly. The automated tests are passing. I'm going to merge this 🚀 |
This PR makes changes similar to orchidjs/sifter.js#33. The primary goal here is to get things working correctly with ESM and TypeScript.
I've tried to carefully think through the potential for breakage, but it's possible something may slip through the cracks. Anything that does break will hopefully be easy to fix forward.
I considered releasing this as a breaking change, but that really shouldn't be necessary; not much should change for consumers. If it does and you find your way to this PR: my sincerest apologies, please open an issue and we'll get things fixed up. TypeScript and ESM are tricky to get right, but this PR should be a large PR in the right direction.