-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
merge upstream & fix dev script #2
Merged
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
I merged manually but haven't pushed to origin, so only have the changes locally until I push. Thanks for sorting, I'll pick this up tomorrow. |
Sorted. Thanks very much. I'm closing this again, and have merged this PR into master manually, and pushed so we should be in sync. |
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.
Hmm. I don't know if this is on purpose or not, but you didn't merge the last PR, just close. So you still have all the commits from my branch in this new PR...
The important commit to fix the dev script issue is this one.
The
"svelte"
field is used by Svelte bundler plugins (e.g. rollup-plugin-svelte) to pick the source of Svelte components."browser"
is usually the field that is picked by bundlers when bundling for browser (as opposed to node, for example). In our case, it seems redundant since all our dists are prod oriented."module"
is the ESM (import
) build that will be used by forward looking codebases."main"
is the default main field; historically a Node entry point. In this project (and the official template it is copied from), it is a UMD build. I don't know anything about it, but I guess it is a Node & browser compatible module format. In our case, there's not a lot we can do in Node since we are publishing DOM (i.e. for browser) components...Appart from the
"main"
field, that is used by Node resolution system, none of this is part of a hard spec. Those are just habits and ad hoc conventions...Anyway, of note here is that our
rollup.config.js
is extracting informations from thepackage.json
. Thename
of the project, but also themodule
andmain
fields, as destinations of the build output:If you change one of those fields to
src/index.js
, then your source file gets overwritten by the build file. On subsequent builds, we're building an already bundledindex.js
, and we get this funny Rollup error. I would laugh if I hadn't played the exact same joke on myself (building on the src) already and lost 2h getting mad at debugging it >_<