Load Rollup configs with import(...) to fix warning
#3840
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Convert the Rollup config files to have a `.mjs` extension so that Node
can natively load them as ES modules and then load them using
`import(...)` rather than the `loadConfigFile` helper.
This avoids a deprecation warning when importing `rollup/dist/loadConfigFile`:
```
(node:21339) [DEP0148] DeprecationWarning: Use of deprecated folder
mapping "./dist/" in the "exports" field module ...
```
This also makes the way the config file is loaded more transparent, since its
just a regular dynamic import.
The downside of this change is that we don't get a `warnings` object
back to print warnings that occur during bundling.
Instead have to pass an `onwarn` handler to `rollup.{rollup, watch}`.
robertknight
added a commit
to hypothesis/lms
that referenced
this pull request
Oct 18, 2021
Convert Rollup configs to native ES modules and load them with `import(...)`. This avoids a deprecation warning from Node triggered by the `loadConfigFile` import. See also hypothesis/client#3840
robertknight
added a commit
to hypothesis/frontend-shared
that referenced
this pull request
Oct 18, 2021
Change bundling in development and when running tests to use Browserify rather than Rollup. This adapts the changes that were recently made in the lms [1] and client [2] projects. [1] hypothesis/lms#3221 [2] hypothesis/client#3840 This follows the pattern and reuses code from the re
robertknight
added a commit
to hypothesis/frontend-shared
that referenced
this pull request
Oct 18, 2021
Change bundling in development and when running tests to use Browserify rather than Rollup. This adapts the changes that were recently made in the lms [1] and client [2] projects. [1] hypothesis/lms#3221 [2] hypothesis/client#3840
lyzadanger
approved these changes
Oct 18, 2021
| import alias from '@rollup/plugin-alias'; | ||
| import babel from '@rollup/plugin-babel'; | ||
| import { babel } from '@rollup/plugin-babel'; |
Comment on lines
+66
to
+68
| if (!Array.isArray(configs)) { | ||
| configs = [configs]; | ||
| } |
There was a problem hiding this comment.
Is there a valid normal condition under which this would occur? Just curious.
There was a problem hiding this comment.
rollup-tests.config.js exports a single object rather than an array. I suppose we could make that export an array as well, but that would be unconventional for a single-output Rollup config.
robertknight
added a commit
to hypothesis/frontend-shared
that referenced
this pull request
Oct 18, 2021
Change bundling in development and when running tests to use Rollup rather than Browserify. This adapts the changes that were recently made in the lms [1] and client [2] projects. [1] hypothesis/lms#3221 [2] hypothesis/client#3840
robertknight
added a commit
to hypothesis/lms
that referenced
this pull request
Oct 18, 2021
Convert Rollup configs to native ES modules and load them with `import(...)`. This avoids a deprecation warning from Node triggered by the `loadConfigFile` import. See also hypothesis/client#3840
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Convert the Rollup config files to have a
.mjsextension so that Nodecan natively load them as ES modules and then load them using
import(...)rather than theloadConfigFilehelper.This avoids a deprecation warning when importing
rollup/dist/loadConfigFile:This also makes the way the config file is loaded more transparent, since its
just a regular dynamic import.
The downside of this change is that we don't get a
warningsobjectback to print warnings that occur during bundling.
Instead have to pass an
onwarnhandler torollup.{rollup, watch}. On the other hand, emitting warnings this way avoids mistakes where the caller forgets to callwarnings.flush().One other side effect of this is that
requirecan no longer be used in the.mjsfiles to load JSON modules. Insteadpackage.jsonhas to be read usingreadFileSyncand parsed usingJSON.parse(related reading). If we're going to start using ES modules for regular Node scripts in future we might as well get used to differences in how certain things have to be written though.