Updating build system for use in CommonJS #141
Conversation
- Separate dist folders for each build. Add overriding package.json file in cjs folder to set `type: 'commonjS'`. (Note: there are difficulties with trying to do this using `cat` in an npm script with {} and some Operating Systems. Simple solution is to copy a file.
- Updated ReadMe
- Added tests to lint script and fixed lint errors
README.md
Outdated
| let parse_tree = jsep("1 + 1"); | ||
| // ESM: | ||
| import { Jsep } from 'jsep'; | ||
| const parse_tree = Jsep.parse('1 + 1'); |
There was a problem hiding this comment.
What is the advantage of this for our users over just using jsep()?
There was a problem hiding this comment.
I'll go back to both. I've been reading tonight a lot about the "evils" of default exports ... so I think our preferred usage should be to use the named export, but I've got it working for both CJS and ESM with both the default and named export now (without needing '.default')
There was a problem hiding this comment.
I always try to keep an open mind, so feel free to send these "evils" my way, but so far I'm entirely unconvinced by the arguments against default exports. The ones I've seen seem to mainly center around inadequacies of tooling. IMHO it's the tooling that needs to be fixed, not default exports that should be avoided. There is a very clear usability benefit to default exports. Not only can the module be imported without looking up its documentation to remember what name to use, but also it offers a clear entry point, that this object is the main thing to use. And the syntax is simpler too. These may seem trivial for experienced developers, but they can make a world of difference to beginners who get easily discouraged and frustrated.
There was a problem hiding this comment.
I was waiting for a response to this, not sure if it still applies?
There was a problem hiding this comment.
ah sorry! I thought it was resolved by providing both named and default exports?
From what I understand:
- using default imports prevents tree-shaking if there are parts of a file that are unused
- mixing named and default exports seems to be discouraged (which is why the build is more difficult for both cjs and esm). But I like that it allows both use cases.
- default imports can lead to inconsistent naming in a code base and may make large-scale refactoring more difficult.
- intellisense may be more problematic with default imports (tooling problem, I know)
- named exports are easier in export aggregation (i.e. barrel files)
In my work, I use a lot of both CJS default and named require statements, but mostly named imports in typescript. (Trying to get away from import * as _ from 'lodash'!). I've found that unit testing named exports can sometimes be easier. But overall, I don't have a strong opinion either way.
README.md
Outdated
| let parse_tree = jsep("1 + 1"); | ||
| // ESM: | ||
| import { Jsep } from 'jsep'; | ||
| const parse_tree = Jsep.parse('1 + 1'); |
There was a problem hiding this comment.
I'll go back to both. I've been reading tonight a lot about the "evils" of default exports ... so I think our preferred usage should be to use the named export, but I've got it working for both CJS and ESM with both the default and named export now (without needing '.default')
- default export now includes static fields. Will allow for removing modifier methods if we like
- default export now adds Jsep as well to support `const { Jsep } = require('jsep')`
- removed eslint disable (switched to arrow functions)
- restored dist file folder structure, and some readme changes
- rollup now does 2 builds so it can use named + default export for ESM, and just the default export for IIFE and CJS (without having to use a .default property)
| <script src="/PATH/TO/jsep.min.js"></script> | ||
| ... | ||
| ```html | ||
| <script src="/PATH/TO/jsep.iife.min.js"></script> |
There was a problem hiding this comment.
I do think we should mention ESM too. They're not only useful for Node. Not sure if they should be before or after IIFE, I'll leave it up to you. I could argue this both ways: ESM should be first because it's the "canonical" version, or IIFE should be first because ESM might scare novices away.
Also, if we're including <script> elements, we should probably include one around the sample code.
update readme example for <script> and ESM
|
Not sure if there's still a change needed @LeaVerou ? Or just waiting for a second review by @EricSmekens ? |
|
Sorry I dropped the ball, thanks for the ping! I approved it now, so you could merge it yourself whenever you want (I'm told this is the new pattern so I figured I'd try it) |
Trying to use jsep in commonjs wasn't working because it was being treated as a module.
type: 'commonjS'. (Note: there are difficulties with trying to do this usingcatin an npm script with {} and some Operating Systems. Simple solution is to copy a file.