Fix unresolved dependencies #823
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
Signed-off-by: Simon Ser <[email protected]>
We don't want to include our dependencies (react, maplibre, and so on) in the final bundle: because library users download these as transitive deps we want to leave it up to them to resolve the imports and include them in the final app bundle. For instance, OSRD is responsible for inlining maplibre imports in ui-warped-map's bundle. On the other hand, relative imports local to the package should be bundled. By default, rollup only resolves relative imports, and doesn't resolve global imports. rollup prints an error when it encounters a global import that it can't resolve. We don't want to just ignore unresolved global imports, because that would silently allow broken non-existing imports. The @rollup/plugin-node-resolve plugin allows rollup to resolve global imports like node does. However, by default it includes all global imports in the bundle. This isn't what we want: we want to exclude dependencies from the bundle. To fix this, follow the recommendations in the rollup docs [1] and use the "external" option to check whether an import points to the "node_modules" directory or a "dist" directory of any osrd-ui package. In that case, ask rollup to not bundle the import. We don't need to manually specify a list of external imports from each package anymore. [1]: https://rollupjs.org/configuration-options/#external Signed-off-by: Simon Ser <[email protected]>
Use the directory name to figure out the current project name. This fixes a bunch of incorrect project names: "osrdcore" was used for packages other than ui-core. Signed-off-by: Simon Ser <[email protected]>
Instead of hand-rolling our own rollup configuration, use the defaults. Fixes unresolved import errors. Signed-off-by: Simon Ser <[email protected]>
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.
See individual commits.
We don't want to include our dependencies (react, maplibre, and so
on) in the osrd-ui library bundle: because library users download these as
transitive deps we want to leave it up to them to resolve the
imports and include them in the final app bundle. For instance,
OSRD is responsible for inlining maplibre imports in ui-warped-map's
bundle.
On the other hand, relative imports local to the package should be
bundled.
By default, rollup only resolves relative imports, and doesn't
resolve global imports. rollup prints an error when it encounters
a global import that it can't resolve. We don't want to just
ignore unresolved global imports, because that would silently
allow broken non-existing imports.
The @rollup/plugin-node-resolve plugin allows rollup to resolve
global imports like node does. However, by default it includes
all global imports in the bundle. This isn't what we want: we
want to exclude dependencies from the bundle.
To fix this, follow the recommendations in the rollup docs
and use the "external" option to check whether an import points
to the "node_modules" directory or a "dist" directory of any
osrd-ui package. In that case, ask rollup to not bundle the
import.
We don't need to manually specify a list of external imports
from each package anymore.