Skip to content
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

fixed: ModuleInfo.importedIds will return null if resolvedIds[source] is undefined #4208

Merged
merged 3 commits into from Aug 23, 2021
Merged

Conversation

@FoxDaxian
Copy link
Contributor

@FoxDaxian FoxDaxian commented Aug 18, 2021

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

Description

I get error (TypeError: Cannot read property 'id' of undefined) when use moduleInfo.importedIds in moduleParsed of rollup plugin

after check importedIds source code, fin not compatible undefined.

moduleInfo.importedIds return null if resolvedIds[source] if undefined
@lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Aug 18, 2021

Can you add a test case where this is wrong? This looks like there is a serious underlying mis-assumption that I would rather fix than hide the symptoms.

@codecov
Copy link

@codecov codecov bot commented Aug 18, 2021

Codecov Report

Merging #4208 (baf94ca) into master (69c099c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4208   +/-   ##
=======================================
  Coverage   98.37%   98.37%           
=======================================
  Files         202      202           
  Lines        7248     7259   +11     
  Branches     2118     2119    +1     
=======================================
+ Hits         7130     7141   +11     
  Misses         58       58           
  Partials       60       60           
Impacted Files Coverage Δ
src/Module.ts 100.00% <100.00%> (ø)
src/ModuleLoader.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 69c099c...baf94ca. Read the comment docs.

@FoxDaxian
Copy link
Contributor Author

@FoxDaxian FoxDaxian commented Aug 19, 2021

I will add test case within whit weekend.

@FoxDaxian
Copy link
Contributor Author

@FoxDaxian FoxDaxian commented Aug 21, 2021

I'm back again~

I have this rollup config:

import testPlugin from './plugin';
import typescript from '@rollup/plugin-typescript';

export default {
    input: './index.ts',
    output: {
        dir: './dist',
        format: 'cjs',
        sourcemap: true,
    },
    plugins: [
        testPlugin(),
        typescript({
            target: 'es5',
        }),
    ],
};

index.ts as below:

import util from './util';
util();

util.ts as below:

const fn = () => {};
export default fn;

testPlugin as below:

export default function (options = {}) {
    return {
        name: 'test-plugin',
        moduleParsed(moduleInfo) {
            console.log(moduleInfo.importedIds);
        },
    };
}

when use moduleInfo.importedIds this preperty will trigger module.info some code as below:

get importedIds() {
    return Array.from(module.sources, source => module.resolvedIds[source].id);
},

and this will report error: TypeError: Cannot read property 'id' of undefined

so I made this pr in order to ignore(or fixed?) this problem.

@lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Aug 22, 2021

The original problem is caused by the fact that moduleParsed is called before all imports of the module have been resolved. Thus your solution would fix the symptom, but the actual ids would be incomplete, most likely an empty array.

There are two possible ways to fix this:

  • Accept that and directly return an empty array in those cases, or
  • Only trigger moduleParsed after all imports have been resolved

I will push a fix for the latter to your branch.

@FoxDaxian
Copy link
Contributor Author

@FoxDaxian FoxDaxian commented Aug 22, 2021

well, after review your commit that make moduleParsed good work.
And, I need continue study rollup harder~
and thanks your reply~

@FoxDaxian FoxDaxian closed this Aug 22, 2021
@FoxDaxian FoxDaxian reopened this Aug 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants