Conversation
229cc25 to
c82fba9
Compare
|
After removing the deprecated options, we have the following configuration options left:
I think this will allow us to give esbuild another try for the next major version:
I would argue that there should only be two options for compact:
We can do that, because we will no longer need I demonstrated how it could like when we go with esbuild here: |
as suggested by @moonglum: this appears to be unmaintained, but it's not really worth the encumbrance anyway
6d7d22a to
1f7df99
Compare
| "dependencies": { | ||
| "@rollup/plugin-commonjs": "~29.0.1", | ||
| "@rollup/plugin-node-resolve": "~16.0.3", | ||
| "browserslist": "^4.28.1", |
There was a problem hiding this comment.
Note that this doesn't add a dependency, but moves the transitive dependency here.
| }); | ||
|
|
||
| it("should take into account Browserslist while transpiling", () => { | ||
| it.skip("should take into account Browserslist while transpiling", () => { |
There was a problem hiding this comment.
I suggest removing both unit tests. They do no longer work, as we are no longer passing in the browsers. Both tests are already covered by the test test/cli/test_browserslist
There was a problem hiding this comment.
🤔 That sounds like a breaking change then? In which case perhaps that should be relegated to v4 as well? (I'm not sure whether the commits' order is intentional: v3.0.3 appears before this change.)
There was a problem hiding this comment.
Nope, that's not a breaking change. Only the way that the unit tests want to use it does not work anymore. But it is not relevant to end users.
FND
left a comment
There was a problem hiding this comment.
Solid work, thank you! I just have a few observations and uncertainties.
Also, I have a vague feeling that commit attribution got lost over the years: Some of these commits are ancient and were moved across multiple branches / release attempts - and I think that for some of them, authorship was altered (e.g. a1b3706 sounds very FNDian). I don't care about attribution per se, but it might lead to confusion in the future - IOW you don't wanna be blamed for my ignorance, do you? Either way, it's not a big deal, mostly a curiosity.
| v3.0.3 | ||
| ------ | ||
|
|
||
| _TBD_ |
There was a problem hiding this comment.
If we deprecate stuff in this release, should we perhaps go with v3.1 instead?
| }); | ||
|
|
||
| it("should take into account Browserslist while transpiling", () => { | ||
| it.skip("should take into account Browserslist while transpiling", () => { |
There was a problem hiding this comment.
🤔 That sounds like a breaking change then? In which case perhaps that should be relegated to v4 as well? (I'm not sure whether the commits' order is intentional: v3.0.3 appears before this change.)
it appears this was never correct in the first place
374ef31 to
0af71a2
Compare
| console.error("WARNING: `format` option is deprecated.\n" + | ||
| "faucet-pipeline-js will always compile to ESM in the future."); |
There was a problem hiding this comment.
For consistency:
| console.error("WARNING: `format` option is deprecated.\n" + | |
| "faucet-pipeline-js will always compile to ESM in the future."); | |
| console.error("WARNING: `format` option is deprecated; " + | |
| "faucet-pipeline-js will always compile to ESM in the future"); |
| var options = { compress: false, mangle: false }; // eslint-disable-line no-var | ||
| break; | ||
| case "mangle": | ||
| console.error("WARNING: compact option `mangle` is deprecated; use `minify` instead"); |
| v3.1.0 | ||
| ------ | ||
|
|
||
| _TBD_ |
There was a problem hiding this comment.
Suggestion, probably incomplete:
| _TBD_ | |
| _2026-0x-xx_ | |
| notable changes for end users: | |
| * deprecated `esnext`, `jsx`, `exports` and `format` options as well as | |
| `compact: mangle` setting | |
| This functionality will be removed in v4 due to lack of user needs: | |
| `esnext` and `format` should no longer be necessary, now that ESM is | |
| well-established. JSX conversion typically requires framework-specific | |
| tooling these days, which goes beyond faucet-pipeline's stated objectives. | |
| `mangle` is deemed excessive and ultimately unhelpful. | |
| * reduced number of dependencies | |
| no significant changes for developers |
| let { abort, repr } = require("faucet-pipeline-core/lib/util"); | ||
| let path = require("path"); | ||
| let browserslist = require("browserslist"); |
There was a problem hiding this comment.
third-party packages above stdlib:
| let { abort, repr } = require("faucet-pipeline-core/lib/util"); | |
| let path = require("path"); | |
| let browserslist = require("browserslist"); | |
| let { abort, repr } = require("faucet-pipeline-core/lib/util"); | |
| let browserslist = require("browserslist"); | |
| let path = require("path"); |
this functionality will be removed in v4 due to lack of user needs: * `esnext`, `format` and `exports` should no longer be necessary, now that ESM is well-established * JSX conversion typically requires framework-specific tooling these days, which goes beyond faucet-pipeline's stated objectives * `compact: mangle` was deemed excessive and ultimately unhelpful; `minify` should be perfectly sufficient Co-authored-by: FND <fnd@localhost.localdomain>
note that we don't actually _need_ faucet-core v3 though because `await` doesn't care whether there's a promise or not
this allows us to remove it from faucet-pipeline-core
|
I think this is ready to release now. At your convenience, can you double-check the change log (WRT both content and phrasing)? |
| "dependencies": { | ||
| "@rollup/plugin-commonjs": "~29.0.1", | ||
| "@rollup/plugin-node-resolve": "~16.0.3", | ||
| "faucet-pipeline-core": "^2.0.0", |
There was a problem hiding this comment.
I would need to double check if with this version, faucet-pipeline-js is compatible with all planned changs for the 3.x branch. Additionally, I think 3.0.0 is the yanked version, so we are probably talking about 3.1.0 at this point? 🤔
https://github.com/faucet-pipeline/faucet-pipeline-core/pulls
| * deprecated `esnext`, `jsx`, `exports` and `format` options as well as | ||
| `compact: mangle` setting | ||
|
|
||
| This functionality will be removed in v4 due to lack of user needs: |
There was a problem hiding this comment.
I'm a bit confused by the way you are presenting this. I suggest something more similar to:
- exports and format should no longer be necessary now that ESM is well-established.
- esnext should no longer be necessary as the "modern" JS syntax has been stable for quite some time and has great browser support.
- jsx has evolved to require more integration with the frameworks. We therefore see it as out of scope for faucet-pipeline.
manglewas deemed excessive and ultimately unhelpful;minifyshould be used instead (offering only reduction of whitespace, not mangling of variable names)
Re-opened PR for #233 as a lot of the changes already got released separately. I also merged in my proposed deprecation warnings.
Note: the JSX and esnext packages should both be marked as deprecated after the release